Thread (2 messages) 2 messages, 2 authors, 2020-08-07

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 */
      }
      #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 ;-)
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 */
+}
 #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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help