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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Aug 7, 2020, 01:32 UTC
Message-ID: <20200807013230.GD8085@camp.crustytoothpaste.net>
In-Reply-To: <xmqqpn83i9sk.fsf@gitster.c.googlers.com>

On 2020-08-07 at 00:23:07, Junio C Hamano wrote:

Show 14 quoted lines

> 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

Maybe "no op" or no-op?

Show 11 quoted lines

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

Great. I was wondering about that when I looked at the patch. If we're not using it, no point in keeping it. I think the name should be "precompose_str", though.

Show 27 quoted lines

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

The rest of the patch looks fine, but do we know that these structs will exist if NO_SETITIMER is defined? If not, we may need to use a void * here, which would provide us worse type checking, but would work on platforms that lack the interval timers at all, such as NonStop.

That does kind of defeat the purpose of this patch, but I still think it's a win, since we end up with some type checking, even if it's not perfect, and almost every platform provides setitimer, so any errors will be caught quickly.

-- 
brian m. carlson: Houston, Texas, US