Re: "#define precompose_argv(c,v) /* empty */" is evil
From: Junio C Hamano <hidden>
Date: 2020-08-07 04:09:42
Jeff King [off-list ref] writes:
But this one for example:quoted
-#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 */
}
#endifYes, 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 ;-)
quoted
@@ -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 */ +} #endifSame 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.