git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>, Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH] list-objects-filter-options: fix function name in BUG
Date: Tue, 17 Nov 2020 17:54:01 -0500	[thread overview]
Message-ID: <20201117225401.GA642410@coredump.intra.peff.net> (raw)
In-Reply-To: <CAN0heSoGnAKjTz2tiHpe2==Y-w7M03eiEpW2hU67FRbv=G+H8w@mail.gmail.com>

On Tue, Nov 17, 2020 at 07:02:19PM +0100, Martin Ågren wrote:

> > But as a general rule, I don't think we even need to include function
> > names here. The message would look like:
> >
> >   BUG: list-objects-filter-options.c:20: list_object_filter_choice_name: invalid argument '3'
> >
> > which already tells us where the code is[1]. Perhaps:
> >
> >   BUG("invalid filter choice enum: %d", c);
> >
> > would be shorter but equally informative (I don't overly care here,
> > since the idea is that nobody sees it, but just making a point about the
> > future).
> 
> Having the function name or something else making the string unique
> across the codebase could be useful if the compiler doesn't support
> variadic macros -- we'll fall back to using a function instead of a
> macro, and can't use __FILE__ and __LINE__. (You obviously know all of
> this, having written d8193743e0 ("usage.c: add BUG() function",
> 2017-05-12).)

My feeling is that we shouldn't care too much about platforms without
variadic macros. AFAIK they are an extreme minority at this point, if
they exist at all. We are being nice by making Git compile at all on
such platforms, but I don't want people doing normal development work
(like writing trace or bug calls) to have to be thinking too much about
it.

I actually wonder if it is time to drop HAVE_VARIADIC_MACROS completely.
They are in C99, and we have been introducing many other C99-isms. It
would be a minor cleanup to a few bits of code, which perhaps isn't
worth the risk. But I also have a vague memory of not being able to
implement some interfaces because we couldn't count on them.

Poking around, 3689539127 (add helpers for allocating flex-array
structs, 2016-02-22) points out one such case. I think discussion of
BUG_ON() got blocked by that, too. Looks like we also discussed them in
the big "C99 weather balloon" thread:

  https://lore.kernel.org/git/20170710070342.txmlwwq6gvjkwtw7@sigill.intra.peff.net/

Maybe it's time for something like this as a test:

diff --git a/git-compat-util.h b/git-compat-util.h
index 2fd9d5b403..fe5de2239f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1194,14 +1194,9 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
 /* usage.c: only to be used for testing BUG() implementation (see test-tool) */
 extern int BUG_exit_code;
 
-#ifdef HAVE_VARIADIC_MACROS
 __attribute__((format (printf, 3, 4))) NORETURN
 void BUG_fl(const char *file, int line, const char *fmt, ...);
 #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
-#else
-__attribute__((format (printf, 1, 2))) NORETURN
-void BUG(const char *fmt, ...);
-#endif
 
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
diff --git a/usage.c b/usage.c
index 06665823a2..b72f48f70e 100644
--- a/usage.c
+++ b/usage.c
@@ -273,23 +273,13 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
 	abort();
 }
 
-#ifdef HAVE_VARIADIC_MACROS
 NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
 	va_start(ap, fmt);
 	BUG_vfl(file, line, fmt, ap);
 	va_end(ap);
 }
-#else
-NORETURN void BUG(const char *fmt, ...)
-{
-	va_list ap;
-	va_start(ap, fmt);
-	BUG_vfl(NULL, 0, fmt, ap);
-	va_end(ap);
-}
-#endif
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS
 void unleak_memory(const void *ptr, size_t len)

  reply	other threads:[~2020-11-17 22:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-14  8:43 Martin Ågren
2020-11-17  2:13 ` Jeff King
2020-11-17 18:02   ` Martin Ågren
2020-11-17 22:54     ` Jeff King [this message]
2020-11-17 23:07       ` Eric Sunshine
2020-11-18  2:08         ` Jeff King
2020-11-18  2:24       ` Jonathan Nieder
2020-11-18  6:22       ` Martin Ågren

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20201117225401.GA642410@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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 inbox:

	https://80x24.org/mirrors/git.git

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