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

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

quoted

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