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

[Jeff King <peff@peff.net>]

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

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