Re: [PATCH] fix some clang warnings

[Jeff King <peff@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.

> 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