git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] list-objects-filter-options: fix function name in BUG
@ 2020-11-14  8:43 Martin Ågren
  2020-11-17  2:13 ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Martin Ågren @ 2020-11-14  8:43 UTC (permalink / raw)
  To: git; +Cc: Taylor Blau

Fix the function name we give in the BUG message. It's "config", not
"choice".

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 list-objects-filter-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index defd3dfd10..d2d1c81caf 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -35,7 +35,7 @@ const char *list_object_filter_config_name(enum list_objects_filter_choice c)
 		/* not a real filter type; just the count of all filters */
 		break;
 	}
-	BUG("list_object_filter_choice_name: invalid argument '%d'", c);
+	BUG("list_object_filter_config_name: invalid argument '%d'", c);
 }
 
 /*
-- 
2.29.2.454.gaff20da3a2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] list-objects-filter-options: fix function name in BUG
  2020-11-14  8:43 [PATCH] list-objects-filter-options: fix function name in BUG Martin Ågren
@ 2020-11-17  2:13 ` Jeff King
  2020-11-17 18:02   ` Martin Ågren
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2020-11-17  2:13 UTC (permalink / raw)
  To: Martin Ågren; +Cc: git, Taylor Blau

On Sat, Nov 14, 2020 at 09:43:26AM +0100, Martin Ågren wrote:

> Fix the function name we give in the BUG message. It's "config", not
> "choice".

Yep, obviously an improvement.

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

-Peff

[1] Obviously it doesn't include the actual function name, though we
    could do so on many platforms by using __FUNCTION__. I tend to think
    it would make the messages overly long, but again, the hope is that
    nobody ever sees these.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] list-objects-filter-options: fix function name in BUG
  2020-11-17  2:13 ` Jeff King
@ 2020-11-17 18:02   ` Martin Ågren
  2020-11-17 22:54     ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Martin Ågren @ 2020-11-17 18:02 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List, Taylor Blau

On Tue, 17 Nov 2020 at 03:13, Jeff King <peff@peff.net> wrote:
>
> On Sat, Nov 14, 2020 at 09:43:26AM +0100, Martin Ågren wrote:
>
> > Fix the function name we give in the BUG message. It's "config", not
> > "choice".
>
> Yep, obviously an improvement.
>
> 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).)

Now, this here BUG shouldn't be a "freak" bug which happens to trigger
under very special circumstances, and where it's not even clear which of
25 equal BUG messages it is that we're seeing. If you add a new enum
value and forget to add a case in this function, you should hit this BUG
quite quickly and very reliably.

All of that said, "don't overly care" also matches my feeling pretty
well.

Martin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] list-objects-filter-options: fix function name in BUG
  2020-11-17 18:02   ` Martin Ågren
@ 2020-11-17 22:54     ` Jeff King
  2020-11-17 23:07       ` Eric Sunshine
                         ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jeff King @ 2020-11-17 22:54 UTC (permalink / raw)
  To: Martin Ågren; +Cc: Git Mailing List, Taylor Blau

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)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] list-objects-filter-options: fix function name in BUG
  2020-11-17 22:54     ` Jeff King
@ 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
  2 siblings, 1 reply; 8+ messages in thread
From: Eric Sunshine @ 2020-11-17 23:07 UTC (permalink / raw)
  To: Jeff King; +Cc: Martin Ågren, Git Mailing List, Taylor Blau

On Tue, Nov 17, 2020 at 5:55 PM Jeff King <peff@peff.net> wrote:
> 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:

There was also an idea[1] to use variadic macros to prevent `errno`
from being clobbered in a case like this:

    die_errno(_("blah %s"), foobar());

In which, if foobar() changes `errno`, then die_errno() reports the wrong value.

[1]: https://lore.kernel.org/git/CAPig+cQKMxwadf9aGyC5ESa-vxDy9PzrYo+m+JaVQ3S=12PyQQ@mail.gmail.com/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] list-objects-filter-options: fix function name in BUG
  2020-11-17 23:07       ` Eric Sunshine
@ 2020-11-18  2:08         ` Jeff King
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2020-11-18  2:08 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Martin Ågren, Git Mailing List, Taylor Blau

On Tue, Nov 17, 2020 at 06:07:07PM -0500, Eric Sunshine wrote:

> On Tue, Nov 17, 2020 at 5:55 PM Jeff King <peff@peff.net> wrote:
> > 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:
> 
> There was also an idea[1] to use variadic macros to prevent `errno`
> from being clobbered in a case like this:
> 
>     die_errno(_("blah %s"), foobar());
> 
> In which, if foobar() changes `errno`, then die_errno() reports the wrong value.
> 
> [1]: https://lore.kernel.org/git/CAPig+cQKMxwadf9aGyC5ESa-vxDy9PzrYo+m+JaVQ3S=12PyQQ@mail.gmail.com/

Thanks, that's another good example. This _could_ be done conditionally
on HAVE_VARIADIC_MACROS, but it would mean on the non-variadic platform
that they'd see the wrong errno. I'm willing to give them worse BUG()
messages, but that may be going too far. :)

I still suspect there may be zero such platforms, which would mean
nobody is affected. But if that is the case, then we should just say so
and get rid of the HAVE_VARIADIC_MACROS flag entirely.

-Peff

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] list-objects-filter-options: fix function name in BUG
  2020-11-17 22:54     ` Jeff King
  2020-11-17 23:07       ` Eric Sunshine
@ 2020-11-18  2:24       ` Jonathan Nieder
  2020-11-18  6:22       ` Martin Ågren
  2 siblings, 0 replies; 8+ messages in thread
From: Jonathan Nieder @ 2020-11-18  2:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Martin Ågren, Git Mailing List, Taylor Blau

Jeff King wrote:

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

Yes.  For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Want to re-send with a signoff?

Thanks,
Jonathan

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] list-objects-filter-options: fix function name in BUG
  2020-11-17 22:54     ` Jeff King
  2020-11-17 23:07       ` Eric Sunshine
  2020-11-18  2:24       ` Jonathan Nieder
@ 2020-11-18  6:22       ` Martin Ågren
  2 siblings, 0 replies; 8+ messages in thread
From: Martin Ågren @ 2020-11-18  6:22 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List, Taylor Blau

On Tue, 17 Nov 2020 at 23:54, Jeff King <peff@peff.net> wrote:
>
> 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/

Ok, that all makes sense.

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

I had a vague memory that for some weather balloons, we would add a
comment like "please let us know if you trip on this; for your own sake,
don't just patch it locally." But maybe that was just for, e.g.,
01d3a526ad ("t0000: check whether the shell supports the "local"
keyword", 2017-10-26), where we add a test, and a single failing test
might otherwise be easy to ignore.

Which is different to what would happen if every file including
git-compat-util.h and/or each invocation of BUG() would make the
compiler complain.

All in all, this diff makes sense.

Martin

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-11-18  6:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-14  8:43 [PATCH] list-objects-filter-options: fix function name in BUG Martin Ågren
2020-11-17  2:13 ` Jeff King
2020-11-17 18:02   ` Martin Ågren
2020-11-17 22:54     ` Jeff King
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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git