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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2020, 00:23 UTC
Message-ID: <xmqqpn83i9sk.fsf@gitster.c.googlers.com>
In-Reply-To: <20200807000126.GC8085@camp.crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 17 quoted lines

> On 2020-08-06 at 23:47:34, Junio C Hamano wrote:
>> I am wondering if it is a good idea to use something like
>> 
>>     static inline void precompose_argv(int argc, const char **argv)
>>     {
>> 	; /* nothing */
>>     }
>> 
>> instead.  As long as the compiler is reasonable enough, this should
>> not result in any code change in the result, except that it would
>> still catch wrong arguments, even if these two parameters are unused
>> and optimized out.
>
> Yes, this seems like a prudent approach.  I believe it's widely used by
> the Linux kernel, so presumably compilers are capable enough to optimize
> it out.  As you noted, it provides type checking for all platforms,
> which is nice.

So I hope the following (untested and not signed off yet) may lead us in the right direction?

-- >8 --
Subject: compat-util: type-check parameters of mocked functions

When there is no need to run a specific function on certain platforms, we often #define an empty function to swallow its parameters and make it into a no-op, e.g.

    #define precompose_argv(c,v) /* no-op */

While this guarantees that no unneeded code is generated, it also discards type and other checks on these parameters, e.g. a new code written with the argv-array API (diff_args is of type "struct argv_array" that has .argc and .argv members):

    precompose_argv(diff_args.argc, diff_args.argv);

must be updated to use "struct strvec diff_args" with .nr and .v members, like so:

    precompose_argv(diff_args.nr, diff_args.v);

after the argv-array API has been updated to the strvec API. However, the "no oop" C preprocessor macro is too aggressive to discard what is unused, and did not catch such a call that was left unconverted.

Using a "static inline" function whose body is a no-op should still result in the same binary with decent compilers yet catch such a reference to a missing field or passing a value of a wrong type.

While at it, I notice that precompute_str() has never been used anywhere in the code, since it was introduced at 76759c7d (git on Mac OS and precomposed unicode, 2012-07-08). Instead of turning it into a static inline, just remove it.

---
 git-compat-util.h | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 5637114b8d..7a0fb7a045 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -252,8 +252,10 @@ typedef unsigned long uintptr_t;
 #ifdef PRECOMPOSE_UNICODE
 #include "compat/precompose_utf8.h"
 #else
-#define precompose_str(in,i_nfd2nfc)
-#define precompose_argv(c,v)
+static inline void precompose_argv(int argc, const char **argv)
+{
+	; /* nothing */
+}
 #define probe_utf8_pathname_composition()
 #endif
 
@@ -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
 
 #ifndef NO_LIBGEN_H
@@ -1231,8 +1235,14 @@ int warn_on_fopen_errors(const char *path);
 #endif
 
 #ifndef _POSIX_THREAD_SAFE_FUNCTIONS
-#define flockfile(fh)
-#define funlockfile(fh)
+static inline void flockfile(FILE *fh)
+{
+	; /* nothing */
+}
+static inline void funlockfile(FILE *fh)
+{
+	; /* nothing */
+}
 #define getc_unlocked(fh) getc(fh)
 #endif