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

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

Jeff King <peff@peff.net> writes:

> 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.

> 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.

>   - 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 ;-)

>> @@ -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.