Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Jeff King <peff@peff.net>]

On Wed, Jan 25, 2017 at 01:28:27PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The only advantage is that it is self-documenting, so somebody does not
> > come through later and convert ("%s", "") back to (""). We could also
> > write a comment. But I am happy if we simply catch it in review (or
> > preferably the person is clueful enough to read the output of git-blame
> > and see why it is that way in the first place).
> 
> And the last sentence unfortunatly does not reflect reality.  
> 
> I would prefer something self-documenting, like your wrapper with a
> comment.  Then somebody who is looking at the implementation of
> warning_blank_line() will not get tempted to turn "%s", "" into ""
> because of the comment.  And somebody who is looking at the callsite
> of warning_blank_line() will think twice before suggesting to turn
> it into warning("").

I am OK with it either way. I was mostly responding to Dscho's
complaint, and I would just like to get this resolved so we never have
to revisit it again. :)

> In any case, the patch is a minimum effort band-aid that lets us
> punt on the whole issue for now, so I'll queue it as-is.

Here's one other option that I came across. Pragmas feel gross, but I
think it will behave as we want, and it doesn't require cooperation from
the callsites at all.

-- >8 --
Subject: [PATCH] disable -Wzero-length-format via #pragma

Building with "gcc -Wall" will complain that the format in:

  warning("")

is empty. Which is true, but the warning is over-eager. We
are calling the function for its side effect of printing
"warning:", even with an empty string.

We disable this warning already with the DEVELOPER Makefile
knob. But we can't unconditionally add -Wno-format-zero-length
to the normal CFLAGS variable, because not all compilers will
understand it. So we may get reports about the warning from
non-developer users who compile with our default of -Wall.

Instead, we can disable the warning using a gcc-specific
#pragma. This should be ignored by non-gcc compilers, and do
what we want for gcc.

I tested also with clang, which often implements gcc
compatible extensions like this. Clang does not generate the
warning in the first place, but also does not complain about
our pragma.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 325950426..a6558930d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -413,6 +413,8 @@ extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+#pragma GCC diagnostic ignored "-Wformat-zero-length"
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
-- 
2.11.0.840.gd37c5973a