Re: [PATCH] fix some clang warnings

From: Jeff King <peff@peff.net>
Date: Jan 16, 2013, 18:00 UTC
Message-ID: <20130116180041.GC27525@sigill.intra.peff.net>
In-Reply-To: <20130116175057.GB27525@sigill.intra.peff.net>

On Wed, Jan 16, 2013 at 09:50:57AM -0800, Jeff King wrote:

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

Ah, I think I see why not.

It is not about the macro itself, but rather the callsites that do not return error, but call it for its printing side effect. It seems that clang -Wunused-value is OK with unused values from functions being discarded, but not with constants. So:

  int foo();
  void bar()
  {
    foo(); /* ok */
    1; /* not ok */
    (foo(), 1); /* not ok */
  }

The first one is OK (I think it would fall under -Wunused-result under either compiler). The middle one is an obvious error, and caught by both compilers. The last one is OK by gcc, but clang complains.

So opterror does not happen to generate any warnings, because we do not ever use it in a void context. It should probably be marked the same way, though, as future-proofing.

Show 6 quoted lines

> The commit message should probably be along the lines of:
> [...]
>   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.

So a more accurate description would be:

  However, clang also defines __GNUC__, but generates warnings with
  -Wunused-value when these macros are used in a void context, because
  the constant "-1" ends up being useless. Gcc does not complain about
  this case (though it is unclear if it is because it is smart enough to
  see what we are doing, or too dumb to realize that the -1 is unused).
  We can squelch the warning by just disabling these macros when clang
  is in use.

-Peff