Re: "#define precompose_argv(c,v) /* empty */" is evil

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2020, 04:09 UTC
Message-ID: <xmqqh7tfhzb1.fsf@gitster.c.googlers.com>
In-Reply-To: <20200807032723.GA15119@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> But this one for example:
>
>> -#define flockfile(fh)
>> -#define funlockfile(fh)
>> +static inline void flockfile(FILE *fh)
>> +{
>> +	; /* nothing */
>> +}
>> +static inline void funlockfile(FILE *fh)
>> +{
>> +	; /* nothing */
>> +}
>
> is re-defining a name that's usually reserved for the system (at least
> on POSIX systems). For most systems that define it, we'd actually use
> the system version (and not compile this code at all).

> But there may be
> systems where we choose not to (either the system version is deficient,
> or we're testing the fallback code on a more-capable system, or our
> #ifndef check isn't sufficient on that system for some reason).

Hmph, perhaps. We'll cross that bridge when we need to port to such a system, and until then, doing this will more easily catch the need to cross that bridge, I would imagine.

Show 14 quoted lines

> If the system defines it as a macro, we'd probably get a garbled
> compiler error as the macro is expanded here. Adding #undef flockfile,
> etc beforehand would help. I'm not sure if the current code might give
> us a macro redefinition warning on such a system.
>
> If the system defines it as a function, we'd probably get redefinition
> warnings.
>
> So...I dunno. Those are all theoretical complaints. But I also think
> what it's buying is not very big:
>
>   - unlike precompose_argv(), modern POSIX-compliant systems (which we
>     all tend to develop on) don't hit this fallback code. So your
>     average developer is likely to see any problems here.

That's OK. I am not particularly interested in systems that has to ifdef out flockfile() and funlockfile(). I did these two primarily to reduce the number of instances of the bad pattern to implement a no-op replacement as C-preprocessor macro that can be copied by less experienced developers.

Show 16 quoted lines

>   - this is really the tip of the portability iceberg anyway. In the
>     example that motivated this thread, you were catching failures to
>     adjust to strvec. But in code like this:
>
>       #ifdef FOO
>       void some_func(int x, const char *y)
>       {
>               struct argv_array whatever = ARGV_ARRAY_INIT;
> 	      ...
>       }
>       #else
>       void some_func(int x, const char *y)
>       {
>               /* do nothing */
>       }
>       #endif

Yes, of course, but as I wrote in my response to Brian, the whole point of using these replacement implementation macros is so that we do not have to sprinkle the main code with such #ifdef/#endif, so I think the code like that is what needs to be corrected ;-)

Show 12 quoted lines

>> @@ -270,7 +272,9 @@ struct itimerval {
>>  #endif
>>  
>>  #ifdef NO_SETITIMER
>> -#define setitimer(which,value,ovalue)
>> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {
>> +	; /* nothing */
>> +}
>>  #endif
>
> Same reasoning applies to this one, plus the added bonus that we'd need
> that struct type defined. brian mentioned using "void *".

See my message to Brian.