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

[Nico Weber]

On Tue, Dec 20, 2011 at 4:07 PM, Chandler Carruth <chandlerc at google.com> 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...

Here's a scenario where I could see the warning to make sense in that
case. Say you have code like this:

if (foo)
  dosomething();
else {
  do();
  more();
  stuff();
}

and then you realize that dosomething() should only happen if bar is
true as well, and you temporarily add some logging to check that:

if (foo)
  if (bar) { printf("here\n"); dosomething(); }
else {
  ...
}

So I'd keep it as is for now, but if turns out to be noisy on your
code base, I'm happy with tweaking / backing it out again.

Nico

>
> Anyway, I'll let others finish up the review that they've already started.
> One nit I saw in glancing through:
>
>    StmtResult ParseStatementOrDeclaration(StmtVector& Stmts,
> -                                         bool OnlyStatement = false);
> +                                         bool OnlyStatement,
> +                                        SourceLocation *TrailingElseLoc =
> NULL);
>
> The indent looks off on this last line...

It's that or 81 cols :-) I saw people doing this in a couple places in
the clang code, so at least it's in good company.

>
>>
>> Nico
>>
>> On Tue, Dec 20, 2011 at 3:28 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> > Indeed!
>> >
>> > On Dec 20, 2011, at 2:45 PM, Matt Beaumont-Gay wrote:
>> >
>> >> On Tue, Dec 20, 2011 at 14:44, Ted Kremenek <kremenek at apple.com> wrote:
>> >>> How about: -Wambigious-else
>> >>
>> >> Preferably spelled "ambiguous" ;)
>> >>
>> >>>
>> >>> On Dec 20, 2011, at 2:38 PM, Matt Beaumont-Gay wrote:
>> >>>
>> >>>> On Tue, Dec 20, 2011 at 14:34, Eli Friedman <eli.friedman at gmail.com>
>> >>>> wrote:
>> >>>>> On Tue, Dec 20, 2011 at 1:36 PM, Nico Weber <thakis at chromium.org>
>> >>>>> wrote:
>> >>>>>> Hi,
>> >>>>>>
>> >>>>>> the attached patch implements a warning for dangling elses, like
>> >>>>>> described at http://drdobbs.com/blogs/cpp/231602010 and as
>> >>>>>> requested
>> >>>>>> in http://llvm.org/pr11609. The warning fires 0 times for chromium
>> >>>>>> and
>> >>>>>> webkit, and it seems to catch a superset of what gcc's
>> >>>>>> -Wparentheses
>> >>>>>> catches.
>> >>>>>>
>> >>>>>> Examples (see the test case for more):
>> >>>>>>
>> >>>>>> This warns:
>> >>>>>> $ cat test.cc
>> >>>>>> void f() {}
>> >>>>>>
>> >>>>>> int main() {
>> >>>>>>  if (false)
>> >>>>>>    if (false)
>> >>>>>>      return 0;
>> >>>>>>  else
>> >>>>>>    return 1;
>> >>>>>> }
>> >>>>>> $ Release+Asserts/bin/clang -c test.cc
>> >>>>>> test.cc:7:3: warning: add explicit braces to avoid dangling else
>> >>>>>> [-Wdangling-else-parentheses]
>> >>>>>>  else
>> >>>>>>  ^
>> >>>>>> 1 warning generated.
>> >>>>>>
>> >>>>>>
>> >>>>>> These don't:
>> >>>>>> $ cat test.cc
>> >>>>>> void f() {}
>> >>>>>>
>> >>>>>> int main() {
>> >>>>>>  if (false) {
>> >>>>>>    if (false)
>> >>>>>>      return 0;
>> >>>>>>  } else
>> >>>>>>    return 1;
>> >>>>>> }
>> >>>>>> $ Release+Asserts/bin/clang -c test.cc
>> >>>>>> $ cat test.cc
>> >>>>>> void f() {}
>> >>>>>>
>> >>>>>> int main() {
>> >>>>>>  if (false)
>> >>>>>>    if (false) {
>> >>>>>>      return 0;
>> >>>>>>    } else
>> >>>>>>      return 1;
>> >>>>>> }
>> >>>>>
>> >>>>> Why exactly do we not want to warn for the following?
>> >>>>>
>> >>>>> int main() {
>> >>>>>  if (false)
>> >>>>>    if (false) {
>> >>>>>      return 0;
>> >>>>>    }
>> >>>>>  else
>> >>>>>    return 1;
>> >>>>> }
>> >>>>>
>> >>>>> Why is the flag called "Wdangling-else-parentheses"?  What do
>> >>>>> parentheses have to do with this warning?
>> >>>>
>> >>>> In GCC, this warning falls into -Wparentheses.
>> >>>>
>> >>>> I'm not super thrilled about the dangling-else-parentheses name
>> >>>> either; I'll try to come up with a constructive suggestion.
>> >>>>
>> >>>> -Matt
>> >>>>
>> >>>> _______________________________________________
>> >>>> cfe-commits mailing list
>> >>>> cfe-commits at cs.uiuc.edu
>> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >>>
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>