<div class="gmail_quote">On Thu, Feb 9, 2012 at 4:17 AM, Dmitri Gribenko <span dir="ltr"><<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hello Chandler,<br>
<br>
Thank you for the review!<br>
<div class="im"><br>
On Thu, Feb 9, 2012 at 11:50 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> I think there should be two ways to silence this warning:<br>
><br>
> 1) place the semicolon for the empty statement on its own line, or<br>
> 2) use empty curly braces.<br>
<br>
</div>(1) always works, </blockquote><div><br></div><div>The very first testcase in your patch shows that it does not always work:</div><div><br></div><div><div>Index: test/Analysis/PR9741.cpp</div><div>===================================================================</div>
<div>--- test/Analysis/PR9741.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 150173)</div><div>+++ test/Analysis/PR9741.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -4,5 +4,5 @@</div><div> int a[] = { 1, 2, 3 };</div><div> unsigned int u = 0;</div><div> for (auto x : a)</div><div>- ;</div><div>+ ; // expected-warning{{range-based for loop has empty body}}</div><div>
}</div></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(2) doesn't work only with range-based for.<br>
<div class="im"><br>
> Secondly, we shouldn't warn on all range-based for loops: they can have<br>
> side-effects the same as for-loops. I think for consistency they should use<br>
> the same rules for silencing the warning.<br>
<br>
</div>I agree that range-based for loops technically can be used only for<br>
side-effects, but that is only possible when iterator's operator++ or<br>
operator!= have side-effects.<br>
<br>
Plain for loops with empty bodies are typically used to advance some<br>
iterator to a certain position. But range-based for loops hide the<br>
iterator from the programmer, so that is not our usecase.<br>
<br>
Thus I think that range-based for loops with empty compound statement<br>
should still warn, because in most cases they are effectively a no-op.<br>
(At least for arrays and STL containers, but I can't think of an<br>
iterator with side-effects useable with a range-based for...)<br></blockquote><div><br></div><div>As Richard has pointed out, there are many ways where one might observe side-effects. However, I think (and he seemed to agree) that the primary motivation is consistency. We should have one rule for all these constructs unless there is an extremely clear reason to have a special case.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> I also don't see a point in warning on switch or if statements when the user<br>
> writes one of the very unlikely to be a bug patterns. It might be as simple<br>
> as them commenting out the actual code, and expecting it to still work. It<br>
> seems very unlikely to be an accidental typo.<br>
<br>
</div>Well, with this patch if(...) {} still doesn't warn because condition<br>
can contain side-effects (only if(...); warns -- but that was<br>
implemented before). Nevertheless, I think that even if with braces<br>
should warn and here's why.<br>
<br>
If the user consciously wrote that code just for the side-effects in<br>
the condition, then `if' statement is the wrong tool. More probably,<br>
either that empty body was a typo or one just forgot to populate the<br>
{} with actual body. If the programmer commented out the body, then<br>
this warning would remind them to uncomment or fix that statement.<br>
<br>
If you have some specific examples that are not handled in a sane way<br>
-- I'm all ears.<br></blockquote><div><br></div><div>I think you're missing my point. I'm trying to make this warning target bugs and mistakes. I don't think it is important or even really advisable for Clang to warn about code which the user very clearly wrote in a particular way even if that formulation is strange or confusing. Quite frankly, it's not the role of the compiler.</div>
<div><br></div><div>The rationale for warning on 'if (...);' iff the following line is indented differently is because it is a very easy typo or mistake to make, not because of any moral objection to empty bodies on ifs.</div>
<div><br></div><div>I'm quite concerned about the following not warning:</div><div><br></div><div>if (...) { /*my_silly_code();*/ }</div><div>if (...) {</div><div> // my_silly_code();</div><div>}</div><div><br></div>
<div>Whether or not you would like the warning on these to remind you to un-comment the code, many do not. If you build with -Werror, this warning becomes useless for you. Even if not, such commented out code should be possible to check in and keep around if needed. It's not my style or convention, but I don't think Clang should get into the business of dictating these patterns.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> Having the same rules for<br>
> silencing a warning on an empty body for all control flow constructs makes<br>
> it much easier to predict how this warning works, which helps users<br>
> understand it.<br>
<br>
</div>I agree, that's why there's only a singe exception from the rules:<br>
range-based for with empty braces.<br>
<div class="im"><br>
> Finally, a few test cases I'd like to see added:<br>
><br>
> switch (x) default:; // should not warn -- used in some metaprogramming<br>
> constructs<br>
><br>
> do {} while (...); // should not warn -- sometimes expanded from macros.<br>
<br>
</div>Done, no code changes.<br>
<div class="im"><br>
> Extra credit if you can handle this, (hey, maybe it already works! it'd be a<br>
> great test case if so...) but if its hard I would do it in a follow-up<br>
> patch. I imagine this QoI issue to be somewhat complicated to handle:<br>
><br>
> do; // should error here due to semi-colon, and synthesize a do-while loop<br>
> x = 42;<br>
> while (...); // should not warn here due to "do" above<br>
<br>
</div>That would not warn unless next line after while(...); has more<br>
indentation than while itself. Added the following testcases:<br>
<br>
do; // expected-note{{to match this 'do'}}<br>
b(); // expected-error{{expected 'while' in do/while loop}}<br>
while (b()); // no-warning<br>
c();<br>
<br>
do; // expected-note{{to match this 'do'}}<br>
b(); // expected-error{{expected 'while' in do/while loop}}<br>
while (b()); // expected-warning{{while loop has empty body}}<br>
expected-note{{put the semicolon on a separate line to silence this<br>
warning}}<br></blockquote><div><br></div><div>I'm specifically asking for this warning to not fire. It's not clear that its going to be correct in the presence of the error before, and it would be better to teach Clang to recover from the error above by associating it with the while here. Again, I'm fine for this to be a FIXME for now.</div>
</div>