[cfe-commits] [Patch] Add a warning for for loops with conditions that do not change

[Douglas Gregor]

On Feb 6, 2012, at 1:00 PM, Eli Friedman wrote:

> On Mon, Feb 6, 2012 at 2:50 PM, Richard Trieu <rtrieu at google.com> wrote:
>> On Mon, Feb 6, 2012 at 2:02 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>> 
>>> On Mon, Feb 6, 2012 at 1:55 PM, Richard Trieu <rtrieu at google.com> wrote:
>>>> The motivation of this path is to catch code like this:
>>>> 
>>>> for (int i = 0; i < 10; ++i)
>>>>   for (int j = 0; j < 10; ++i)
>>>>     { }
>>>> 
>>>> The second for loop increments i instead of j causing an infinite loop.
>>>>  The
>>>> warning also checks the body of the for loop so it will trigger on:
>>>> 
>>>> for (int i; i <10; ) { }
>>>> 
>>>> But not trigger on:
>>>> 
>>>> for (int i; i< 10; ) { ++i; }
>>>> 
>>>> I'm still fine-tuning the trigger conditions, but would like some
>>>> feedback
>>>> on this patch.
>>> 
>>> Adding an additional recursive traversal of the body of every parsed
>>> for loop is very expensive.
>>> 
>>> -Eli
>> 
>> 
>> Is it that expensive?  I would think that once the AST was constructed, the
>> visitor would be pretty fast, especially if no action is taken on most of
>> the nodes.  I also made the warning default ignore and put in an early
>> return to prevent the visitors from running in the default case.
>> 
>> Do you have any suggestions on removing the recursive traversal?
> 
> Okay, thinking about it a bit more, maybe it's not that expensive, but
> you should at least measure to make sure.

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.

> I don't have any good suggestion for an alternative.


Nor do I.

A couple more comments:

+    ValueDecl *VD = E->getDecl();
+    for (SmallVectorImpl<ValueDecl*>::iterator I = Decls.begin(),
+                                               E = Decls.end();
+         I != E; ++I)
+      if (*I == VD) {
+        FoundDecl = true;
+        return;
+      }

This is linear; please use a SmallPtrSet instead.

Plus, I think you want to narrow this check to only consider VarDecls. Functions and enumerators are not interesting.

+      S.Diag(Second->getLocStart(), diag::warn_variables_not_in_loop_body)
+          << Second->getSourceRange();

This warning needs to specify which variables were not used or point to them in the source.

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?

	- Doug