Re: [PATCH 9/6] git-compat-util: guard definition of MAYBE_UNUSED with __GNUC__

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 29, 2024 at 11:27:39AM -0700, Junio C Hamano wrote:
>
>> Just like we only define UNUSED macro when __GNUC__ is defined,
>> and fall back to an empty definition otherwise, we should do the
>> same for MAYBE_UNUSED.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  * Before I forget that we have discussed this, just as a
>>    documentation (read: this is not a patch to be applied).
>> 
>>    I think this only matters when a compiler satisfies all three
>>    traits:
>> 
>>    - does not define __GNUC__
>>    - does have its own __attribute__() macro
>>    - barfs on __attribute__((__unused__))
>> 
>>    Otherwise we will define __attribute__(x) away to empty to cause
>>    no harm.
>> 
>>    Since we have survived without complaints without such a guard
>>    for quite some time, it may be a sign that no compiler that knows
>>    __attribute__() that people ever tried to compile Git with barfs
>>    with __attribute__((__unused__)).  I dunno.
>
> Yeah, I was surprised that this didn't have a guard and was not
> currently barfing on other compilers. And the answer is that we already
> turn __attribute__ into a noop on non-GNUC platforms.

Plus these non-GNUC platforms either

 (1) do not have their own __attribute__, which lets us turn
     __attribute__() into noop, or

 (2) have their own __attribute__, but they happen to support
     __attribute__((__unused__)).

If somebody has __attribute__() and does not support (__unused__) in
it, use of MAYBE_UNUSED would be broken (maybe their __attribute__()
supports other things but not unused).

> Which made me wonder if UNUSED really needs its guards. It does, because
> it is defined early in the file, before the __attribute__ handling. I
> don't think we want to move it down, since it needs to be available for
> use by inline'd compat wrappers. But arguably we should move the
> attribute macro earlier in the file?

And moving __attribute__ definition earlier in the file would not
help such a platform with broken __attribute__((__unused__))

> I don't know that it is really worth spending too much time futzing
> with, though.

I am inclined to think it is not.  So let's scrap the patch.  The
list archive will hopefully remember when it becomes necessary ;-)