Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

From: Jeff King <peff@peff.net>
Date: Nov 22, 2017, 23:37 UTC
Message-ID: <20171122233709.GB8577@sigill>
In-Reply-To: <20171122230239.GG11671@aiede.mtv.corp.google.com>

On Wed, Nov 22, 2017 at 03:02:39PM -0800, Jonathan Nieder wrote:

Show 18 quoted lines

> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
> >  __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__)
> > +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while (0)
> >  #else
> >  __attribute__((format (printf, 1, 2))) NORETURN
> >  void BUG(const char *fmt, ...);
> > +
> > +__attribute__((format (printf, 2, 3)))
> > +void BUG_ON(int condition, const char *fmt, ...);
> >  #endif
> 
> I worry that these definitions are mildly incompatible: the macro
> accepts anything that can go in an 'if', including pointers, and the
> function only accepts an int.

I suspect this would cause real latent problems. Doing:

  const char *foo = NULL;

  ...
  BUG_ON(foo, "foo was set twice!");
  foo = xstrdup(bar);

would compile fine on modern systems, but issue a pointer-to-int conversion warning if you didn't have variadic macros. So most git devs would be unlikely to see it, until the trap springs on some poor soul building on ancient AIX or something.

On the other hand, I wouldn't be surprised if our friends on less-abled compilers see a lot of pointless warnings anyway, and almost certainly can't deal with the equivalent of "-Werror".

I'm also not sure which compilers that even encompasses these days. Maybe we should add variadic macros to our list of weather-balloon c99 features to test for.

> Is there a way for the macro to typecheck that its argument is an
> integer to avoid that?

I couldn't think of one.

It would be nice to just "!!condition" in the non-macro case, but of course we can't do that automatically without a macro.

You're asking for the opposite: force the macro version to require an int, so that the caller has to remember to do "!!condition". I can't think of a way to do that, but I'm also not sure I like the direction, as it pushes the effort into the callsite.

-Peff