mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: "Ævar Arnfjörð Bjarmason" <>
Cc: Bagas Sanjaya <>,
	Junio C Hamano <>,,
	Fabrice Fontaine <>
Subject: Re: [PATCH] git-compat-util: avoid redefining system function names
Date: Fri, 2 Dec 2022 17:50:18 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

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

> > +#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;
-- 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.


      reply	other threads:[~2022-12-02 22:51 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-25  9:23 [PATCH] git-compat-util.h: Fix build without threads Bagas Sanjaya
2022-11-25 23:47 ` Ævar Arnfjörð Bjarmason
2022-11-28  5:04   ` Jeff King
2022-11-29  3:30   ` Bagas Sanjaya
2022-11-29  3:46     ` Bagas Sanjaya
2022-11-28  5:01 ` Jeff King
2022-11-30 21:15   ` [PATCH] git-compat-util: avoid redefining system function names Jeff King
2022-12-02 10:05     ` Bagas Sanjaya
2022-12-02 11:05       ` Jeff King
2022-12-03  8:02         ` Bagas Sanjaya
2022-12-07  8:33           ` Bagas Sanjaya
2022-12-07 13:00             ` Jeff King
2022-12-02 11:31     ` Ævar Arnfjörð Bjarmason
2022-12-02 22:50       ` Jeff King [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).