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

[Chandler Carruth]

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

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


> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111220/5646ba94/attachment.html>