[cfe-commits] [patch] Add -Wdangling-else-parentheses

[Richard Smith]

Hi,

On Wed, December 21, 2011 00:07, Chandler Carruth wrote:
> On Tue, Dec 20, 2011 at 3:58 PM, Nico Weber <thakis at chromium.org> wrote:
>> New version!
>>
>> This warns on the example Eli gave (like gcc does), and uses
>> -Wdangling-else as suggested by nbjoerg on IRC (part of -Wall)
>>
>> espindola tells me that this warning fires 0 times with a Firefox build. It
>> now has 1 false positive in chrome on code that looks like
>>
>> if (...) for (...) if (...) { } else {
>> }
>>
>> but I can live with that.
>
> Ewww. =/ I really don't like missing this, and it seems like we should be
> able to handle it... All we would need to do is track whether the else was
> followed by a '{', no? Would that cause a lexing performance problem? I
> thought peaking at the token stream by one token was cheap, but maybe not...

I don't feel comfortable with calling this a false positive. This code is
still ambiguous, in the sense that the 'else' could have been intended to pair
up with either 'if'.

However, based on GCC's history of not diagnosing this case, there is a
significant quantity of real-world code which uses constructs like this in
order to hide the warning on if-else constructs which come from inside macros:

  #define FOO(X) \
    switch (0) default: \
      if (!(X)) \
        HandleDisabledThing(); \
      else
        GetThing()

  // ...

  if (cond)
    FOO(x) << "hello"; // no warning

After some discussion off-line with Chandler, we have agreed that we think the
warning should be suppressed if:

 - Both the 'else' and the associated 'if' come from the same macro expansion
 - The non-associated 'if' does not come from that macro expansion.

Thanks,
Richard