<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>I also did a comparison between runs with and without -Wloop-analysis. Even with the heavily nested loop, the extra recursive checks don't take too much extra time to run.</div><div><br></div><div>clang loop.cc -fsyntax-only</div>
<div>.460 - .480s</div><div><br></div><div>clang loop.cc -fsyntax-only -Wloop-analysis</div><div>.520 - .540s</div><div><br>
</div><div>Code with increments removed to trigger 2044 for loop warnings</div><div>clang loop.cc -fsyntax-only</div><div>.310 - .330s</div><div><br></div><div>clang loop.cc -fsyntax-only -Wloop-analysis 2>/dev/null</div>
<div>.740 - .780s</div><div><br></div><div>clang loop.cc -fsyntax-only -Wloop-analysis</div>
<div>1.430 - 1.550s</div><div><br></div><div>// Test loop code.</div><div><div>#define M(A) A A</div><div>#define L1 for(int a1 = 0; a1 < 10;){M(L2) }</div><div>#define L2 for(int a2 = 0; a2 < 10;){M(L3) a1++; }</div>
<div>#define L3 for(int a3 = 0; a3 < 10;){M(L4) a2++; }</div><div>#define L4 for(int a4 = 0; a4 < 10;){M(L5) a3++; }</div><div>#define L5 for(int a5 = 0; a5 < 10;){M(L6) a4++; }</div><div>#define L6 for(int a6 = 0; a6 < 10;){M(L7) a5++; }</div>
<div>#define L7 for(int a7 = 0; a7 < 10;){M(L8) a6++; }</div><div>#define L8 for(int a8 = 0; a8 < 10;){M(L9) a7++; }</div><div>#define L9 for(int a9 = 0; a9 < 10;){M(L) a8++; }</div><div>#define L a9++;</div><div>
<br></div><div>void foo() {</div><div> L1</div><div> L1</div><div> L1</div><div> L1</div><div>}</div></div>