Re: [PATCH] git-compat-util: avoid redefining system function names

From: Jeff King <peff@peff.net>
Date: Dec 2, 2022, 22:50 UTC
Message-ID: <Y4qBKoIpKLkthQXb@coredump.intra.peff.net>
In-Reply-To: <221202.86wn7af2xl.gmgdl@evledraar.gmail.com>

On Fri, Dec 02, 2022 at 12:31:32PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 17 quoted lines

> > But with a little work, we can have our cake and eat it, too. If we
> > define the type-checking wrappers with a unique name, and then redirect
> > the system names to them with macros, we still get our type checking,
> > but without redeclaring the system function names.
> 
> This looks good to me. The only thing I'd like to note is that while the
> above explanation might read as though this is something novel, it's
> really just doing exactly what we're already doing for
> e.g. git_vsnprintf:
> 	
> 	$ git -P grep git_snprintf
> 	compat/snprintf.c:int git_snprintf(char *str, size_t maxsize, const char *format, ...)
> 	git-compat-util.h:#define snprintf git_snprintf
> 	git-compat-util.h:int git_snprintf(char *str, size_t maxsize,
> 
> Now, that's not a downside here but an upside, plain old boring and
> using existing precedence is a goood thing. Except that....

Yes, though the motivation here is just a tiny bit different. In the case of snprintf, say, the reason we intercept it is that we know the platform version sucks and we want to replace it. With the functions touched by 15b52a44e0, the idea was that we're replacing them because the platform doesn't provide them at all, and so macros weren't needed. But that assumption turns out sometimes not to be true.

Show 7 quoted lines

> > +#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue)
> 
> ...this part is where we differ from the existing pattern. I don't think
> it matters except for the redundant verbosity, but as we're not
> re-arranging the parameters here, why not simply:
> 
> 	#define setitimer git_setitimer

The two aren't quite the same. Try this:

-- >8 --
gcc -E - <<-\EOF
#define no_parens replaced
#define with_parens(x) replaced(x)
struct foo {
  no_parens x;
  with_parens x;
};
no_parens(foo);
with_parens(foo);
EOF
-- 8< --

If the macro is defined with parentheses, it's replaced only in function-call contexts. Whereas without it, any token is replaced. I doubt it matters much in the real world either way. Replacing only in function context seems to match the intent a bit more, though note that if you tried to take a function pointer to a macro-replaced name, you'd get the original.

As you saw, there are many examples of each style, and I don't think we've ever expressed a preference for one or the other.

Note that for some, like snprintf, we traditionally _had_ to use the non-function form because we couldn't count on handling varargs correctly. In the opposite direction, many macros have to use the function form because they modify the arguments (e.g., foo(x) pointing to repo_foo(the_repository, x)).

> I went looking a bit more, and we also have existing examples of these
> sort of macros, but could probably have this on top of "next" if we care
> to make these consistent.

I don't have a strong feeling either way. I think you'd need to argue in the commit message why one form is better than the other. The function pointer thing is probably the most compelling to me.

-Peff