Re: [PATCH] fix some clang warnings

[Jeff King <peff@peff.net>]

On Wed, Jan 16, 2013 at 06:26:35PM +0100, Max Horn wrote:

> > On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
> >> FWIW, I also happen to have the warning:
> >> 
> >> advice.c:69:2: warning: expression result unused [-Wunused-value]
> >>        error("'%s' is not possible because you have unmerged files.", me);
> >>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> ./git-compat-util.h:314:55: note: expanded from:
> >> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
> >>                                                      ^~
> >> 
> >> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
> >> (based on LLVM 3.0)
> > 
> > I have the same output with:
> > 
> > clang version 3.2 (tags/RELEASE_32/final)
> 
> Sorry for not being more specific in my message. I have this with 
> 
> Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)

So it seems pretty common, and is just that clang is more concerned
about this than gcc. I think your patch is a reasonable workaround. It
seems a little weird to me that clang defines __GNUC__, but I assume
there are good reasons for it. The commit message should probably be
along the lines of:

  Commit a469a10 wraps some error calls in macros to give the compiler a
  chance to do more static analysis on their constant -1 return value.
  We limit the use of these macros to __GNUC__, since gcc is the primary
  beneficiary of the new information, and because we use GNU features
  for handling variadic macros.

  However, clang also defines __GNUC__, but generates warnings (due to
  throwing away the return value from the first half of the macro). We
  can squelch the warning by turning off these macros when clang is in
  use.

I'm confused, though, why your patch does not have a matching update to
the opterror macro in parse-options.h. It uses exactly the same
technique. Does it not generate a warning?

-Peff