<div class="gmail_quote">On Mon, Mar 12, 2012 at 1:52 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div class="h5"><div>On Mar 12, 2012, at 1:38 PM, Richard Trieu wrote:</div><br><blockquote type="cite"><br><br><div class="gmail_quote">On Sun, Mar 11, 2012 at 1:58 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div><div>On Feb 14, 2012, at 3:42 PM, Richard Trieu wrote:</div><br></div></div><blockquote type="cite"><div><div><div class="gmail_quote">On Tue, Feb 7, 2012 at 10:38 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
On Feb 6, 2012, at 1:00 PM, Eli Friedman wrote:<br>
<br>
> On Mon, Feb 6, 2012 at 2:50 PM, Richard Trieu <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
>> On Mon, Feb 6, 2012 at 2:02 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>> wrote:<br>
>>><br>
>>> On Mon, Feb 6, 2012 at 1:55 PM, Richard Trieu <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
>>>> The motivation of this path is to catch code like this:<br>
>>>><br>
>>>> for (int i = 0; i < 10; ++i)<br>
>>>> for (int j = 0; j < 10; ++i)<br>
>>>> { }<br>
>>>><br>
>>>> The second for loop increments i instead of j causing an infinite loop.<br>
>>>> The<br>
>>>> warning also checks the body of the for loop so it will trigger on:<br>
>>>><br>
>>>> for (int i; i <10; ) { }<br>
>>>><br>
>>>> But not trigger on:<br>
>>>><br>
>>>> for (int i; i< 10; ) { ++i; }<br>
>>>><br>
>>>> I'm still fine-tuning the trigger conditions, but would like some<br>
>>>> feedback<br>
>>>> on this patch.<br>
>>><br>
>>> Adding an additional recursive traversal of the body of every parsed<br>
>>> for loop is very expensive.<br>
>>><br>
>>> -Eli<br>
>><br>
>><br>
>> Is it that expensive? I would think that once the AST was constructed, the<br>
>> visitor would be pretty fast, especially if no action is taken on most of<br>
>> the nodes. I also made the warning default ignore and put in an early<br>
>> return to prevent the visitors from running in the default case.<br>
>><br>
>> Do you have any suggestions on removing the recursive traversal?<br>
><br>
> Okay, thinking about it a bit more, maybe it's not that expensive, but<br>
> you should at least measure to make sure.<br>
<br>
</div></div>I'm still very nervous about adding such an AST traversal. Presumably, it's not going to be a concern in practice because most of the time, the increment statement of the 'for' loop will mention one of the variables in the condition, and therefore we'll short-circuit the expensive walk of the loop body. It would be helpful to know that's the case.<br>
<div><br>
> I don't have any good suggestion for an alternative.<br>
<br>
<br>
</div>Nor do I.<br>
<br>
A couple more comments:<br>
<br>
+ ValueDecl *VD = E->getDecl();<br>
+ for (SmallVectorImpl<ValueDecl*>::iterator I = Decls.begin(),<br>
+ E = Decls.end();<br>
+ I != E; ++I)<br>
+ if (*I == VD) {<br>
+ FoundDecl = true;<br>
+ return;<br>
+ }<br>
<br>
This is linear; please use a SmallPtrSet instead.<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Plus, I think you want to narrow this check to only consider VarDecls. Functions and enumerators are not interesting.<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ S.Diag(Second->getLocStart(), diag::warn_variables_not_in_loop_body)<br>
+ << Second->getSourceRange();<br>
<br>
This warning needs to specify which variables were not used or point to them in the source.<br></blockquote><div>The diagnostic now underlines all the variables in the condition. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Do we want to actually look for modification, e.g., any use of the variable that isn't immediately consumed by an lvalue-to-rvalue conversion?<br></blockquote><div>Yes, that is exactly what we are checking for. The VisitCastExpr checks for LvalueToRvalue casts and skips any DeclRefExpr's that are direct sub-expressions.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- Doug<br>
</blockquote></div><br>
</div></div><div>I also did a comparison between runs with and without -Wloop-analysis. [snip]</div></blockquote><br></div><div>Thanks for all of the performance testing. A few more comments on the actual code:</div><div>
<br></div><div><div>Index: include/clang/Basic/DiagnosticSemaKinds.td</div><div>===================================================================</div><div>--- include/clang/Basic/DiagnosticSemaKinds.td<span style="white-space:pre-wrap"> </span>(revision 150501)</div>
<div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -14,6 +14,11 @@</div><div> let Component = "Sema" in {</div><div> let CategoryName = "Semantic Issue" in {</div>
<div> </div><div>+// For loop analysis</div><div>+def warn_variables_not_in_loop_body : Warning<</div><div>+ "variables used in loop condition not modified in loop body">,</div><div>+ InGroup<DiagGroup<"loop-analysis">>, DefaultIgnore;</div>
<div><br></div><div>It would be really nice if this diagnostic could mention the variables by name ("variables 'i' and 'n' used in loop condition are not modified in loop body"), like we do with missing enumerators in a switch. It'll take some special casing for 1/2/many variables, but it will be much nicer for the user. </div>
</div></div></blockquote><div><br></div><div>I will look into this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div></div><div><div>+ void VisitDeclRefExpr(DeclRefExpr *E) {</div><div>+ VarDecl *VD = dyn_cast<VarDecl>(E->getDecl());</div><div>+ if (!VD) return;</div><div>+</div><div>+ PDiag << E->getSourceRange();</div>
<div>+</div><div>+ // Dont do analysis on pointers.</div><div>+ if (VD->getType()->isAnyPointerType()) {</div><div>+ Simple = false;</div><div>+ return;</div><div>+ }</div><div>+</div><div>+ Decls.insert(VD);</div>
<div>+ }</div><div><br></div><div>Presumably, anything volatile-qualified should make the expression non-simple.</div></div></div></div></blockquote><div>The check for volatile is performed after the DeclMatcher finishes.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div><br></div></div><div><div>+ void VisitCastExpr(CastExpr *E) {</div><div>
+ // Don't do analysis on function calls or arrays.</div><div>+ if (E->getCastKind() == CK_FunctionToPointerDecay ||</div><div>+ E->getCastKind() == CK_ArrayToPointerDecay) {</div><div>+ Simple = false;</div>
<div>+ return;</div><div>+ }</div><div>+</div><div>+ Visit(E->getSubExpr());</div><div>+ }</div><div><br></div><div>If you want to avoid calls, why not just intercept CallExpr and CXXConstructExpr and call those non-simple?</div>
<div><br></div><div>Backing up a step, it seems strange that DeclExtractor assumes that an expression is 'simple' unless it sees some very small number of cases that aren't consider simple. That feels very brittle to me because, e.g., CXXConstructExpr is certainly not simple (for non-trivial constructors), yet it would be considered simple by this approach. Similarly for CXXNewExpr and a host of other non-trivial expressions that aren't covered. If we have to have the notion of 'simple', then we need to build it constructively by white-listing expressions that are known to be simple and banning (by default) all other expressions.</div>
<div><br></div></div></div></div></blockquote><div>I'm a bit worried about how many visitor methods I need to write to, but I'll give it a try and see what happens.</div></div></blockquote><div><br></div></div></div>
<div>I could settle for a full audit of all of the expressions, but the fact that there are several obvious missing cases worries me a lot.</div><div class="im"><br><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div></div><div><div>+ // DeclMatcher checks to see if the decls are used in a non-evauluated</div><div>+ // context. </div><div>+ class DeclMatcher : public StmtVisitor<DeclMatcher> {</div>
<div><br></div><div>Should this use an EvaluatedExprVisitor to provide more accurate diagnostics? e.g., the presence of sizeof(x) doesn't mean that 'x' has been used in a meaningful way.</div></div></div></div>
</div></blockquote><div><br></div><div>An EvaluatedExprVisitor is used to find the decls used in the loop condition, which is called the DeclExtractor. A separate visitor, the DeclMatch, is used on the loop conditional, the loop increment, and the loop body to see if the decls are in use. For this, non-evaluated contexts that could change the decl are important, so an EvaluatedExprVisitor can not be used here.</div>
</div></blockquote><br></div></div><div>How would a non-evaluated context change the value of the variable?</div><div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><div><br></div><br></div>
</blockquote></div>Oops, I seemed to have taken part of this conversation off list. I was a little fuzzy on the definition of an evaluated context, and a bit of testing shows that an EvaluatedExprVisitor will work for this warning. I will drop the StmtVisitor.