Re: "#define precompose_argv(c,v) /* empty */" is evil
From: brian m. carlson <hidden>
Date: 2020-08-07 01:33:09
Possibly related (same subject, not in this thread)
- 2020-08-07 · Re: "#define precompose_argv(c,v) /* empty */" is evil · Torsten Bögershausen <hidden>
- 2020-08-07 · Re: "#define precompose_argv(c,v) /* empty */" is evil · Junio C Hamano <hidden>
- 2020-08-07 · Re: "#define precompose_argv(c,v) /* empty */" is evil · Jeff King <hidden>
- 2020-08-07 · Re: "#define precompose_argv(c,v) /* empty */" is evil · Junio C Hamano <hidden>
- 2020-08-07 · Re: "#define precompose_argv(c,v) /* empty */" is evil · Junio C Hamano <hidden>
On 2020-08-07 at 00:23:07, Junio C Hamano wrote:
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 toMaybe "no op" or no-op?
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.
quoted hunk ↗ jump to hunk
--- 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
Attachments
- signature.asc [application/pgp-signature] 263 bytes