<div class="gmail_quote">On Tue, Dec 20, 2011 at 3:58 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
New version!<br>
<br>
This warns on the example Eli gave (like gcc does), and uses<br>
-Wdangling-else as suggested by nbjoerg on IRC (part of -Wall)<br>
<br>
espindola tells me that this warning fires 0 times with a Firefox<br>
build. It now has 1 false positive in chrome on code that looks like<br>
<br>
  if (...)<br>
    for (...)<br>
      if (...) {<br>
      } else {<br>
      }<br>
<br>
but I can live with that.<br></blockquote><div><br>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...</div>
<div><br></div><div>Anyway, I'll let others finish up the review that they've already started. One nit I saw in glancing through:</div><div><br></div><div><div>   StmtResult ParseStatementOrDeclaration(StmtVector& Stmts,</div>
<div>-                                         bool OnlyStatement = false);</div><div>+                                         bool OnlyStatement,</div><div>+                                        SourceLocation *TrailingElseLoc = NULL);</div>
</div><div><br></div><div>The indent looks off on this last line...</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
Nico<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Tue, Dec 20, 2011 at 3:28 PM, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br>
> Indeed!<br>
><br>
> On Dec 20, 2011, at 2:45 PM, Matt Beaumont-Gay wrote:<br>
><br>
>> On Tue, Dec 20, 2011 at 14:44, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br>
>>> How about: -Wambigious-else<br>
>><br>
>> Preferably spelled "ambiguous" ;)<br>
>><br>
>>><br>
>>> On Dec 20, 2011, at 2:38 PM, Matt Beaumont-Gay wrote:<br>
>>><br>
>>>> On Tue, Dec 20, 2011 at 14:34, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> wrote:<br>
>>>>> On Tue, Dec 20, 2011 at 1:36 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
>>>>>> Hi,<br>
>>>>>><br>
>>>>>> the attached patch implements a warning for dangling elses, like<br>
>>>>>> described at <a href="http://drdobbs.com/blogs/cpp/231602010" target="_blank">http://drdobbs.com/blogs/cpp/231602010</a> and as requested<br>
>>>>>> in <a href="http://llvm.org/pr11609" target="_blank">http://llvm.org/pr11609</a>. The warning fires 0 times for chromium and<br>
>>>>>> webkit, and it seems to catch a superset of what gcc's -Wparentheses<br>
>>>>>> catches.<br>
>>>>>><br>
>>>>>> Examples (see the test case for more):<br>
>>>>>><br>
>>>>>> This warns:<br>
>>>>>> $ cat test.cc<br>
>>>>>> void f() {}<br>
>>>>>><br>
>>>>>> int main() {<br>
>>>>>>  if (false)<br>
>>>>>>    if (false)<br>
>>>>>>      return 0;<br>
>>>>>>  else<br>
>>>>>>    return 1;<br>
>>>>>> }<br>
>>>>>> $ Release+Asserts/bin/clang -c test.cc<br>
>>>>>> test.cc:7:3: warning: add explicit braces to avoid dangling else<br>
>>>>>> [-Wdangling-else-parentheses]<br>
>>>>>>  else<br>
>>>>>>  ^<br>
>>>>>> 1 warning generated.<br>
>>>>>><br>
>>>>>><br>
>>>>>> These don't:<br>
>>>>>> $ cat test.cc<br>
>>>>>> void f() {}<br>
>>>>>><br>
>>>>>> int main() {<br>
>>>>>>  if (false) {<br>
>>>>>>    if (false)<br>
>>>>>>      return 0;<br>
>>>>>>  } else<br>
>>>>>>    return 1;<br>
>>>>>> }<br>
>>>>>> $ Release+Asserts/bin/clang -c test.cc<br>
>>>>>> $ cat test.cc<br>
>>>>>> void f() {}<br>
>>>>>><br>
>>>>>> int main() {<br>
>>>>>>  if (false)<br>
>>>>>>    if (false) {<br>
>>>>>>      return 0;<br>
>>>>>>    } else<br>
>>>>>>      return 1;<br>
>>>>>> }<br>
>>>>><br>
>>>>> Why exactly do we not want to warn for the following?<br>
>>>>><br>
>>>>> int main() {<br>
>>>>>  if (false)<br>
>>>>>    if (false) {<br>
>>>>>      return 0;<br>
>>>>>    }<br>
>>>>>  else<br>
>>>>>    return 1;<br>
>>>>> }<br>
>>>>><br>
>>>>> Why is the flag called "Wdangling-else-parentheses"?  What do<br>
>>>>> parentheses have to do with this warning?<br>
>>>><br>
>>>> In GCC, this warning falls into -Wparentheses.<br>
>>>><br>
>>>> I'm not super thrilled about the dangling-else-parentheses name<br>
>>>> either; I'll try to come up with a constructive suggestion.<br>
>>>><br>
>>>> -Matt<br>
>>>><br>
>>>> _______________________________________________<br>
>>>> cfe-commits mailing list<br>
>>>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>>><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br>