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

[Richard Trieu]

On Tue, Feb 7, 2012 at 10:38 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> 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.
>
Done.

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

>
> +      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.
>
The diagnostic now underlines all the variables in the condition.

>
> 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?
>
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.

>
>        - Doug
>

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.

clang loop.cc -fsyntax-only
.460 - .480s

clang loop.cc -fsyntax-only -Wloop-analysis
.520 - .540s

Code with increments removed to trigger 2044 for loop warnings
clang loop.cc -fsyntax-only
.310 - .330s

clang loop.cc -fsyntax-only -Wloop-analysis 2>/dev/null
.740 - .780s

clang loop.cc -fsyntax-only -Wloop-analysis
1.430 - 1.550s

// Test loop code.
#define M(A) A A
#define L1 for(int a1 = 0; a1 < 10;){M(L2) }
#define L2 for(int a2 = 0; a2 < 10;){M(L3) a1++; }
#define L3 for(int a3 = 0; a3 < 10;){M(L4) a2++; }
#define L4 for(int a4 = 0; a4 < 10;){M(L5) a3++; }
#define L5 for(int a5 = 0; a5 < 10;){M(L6) a4++; }
#define L6 for(int a6 = 0; a6 < 10;){M(L7) a5++; }
#define L7 for(int a7 = 0; a7 < 10;){M(L8) a6++; }
#define L8 for(int a8 = 0; a8 < 10;){M(L9) a7++; }
#define L9 for(int a9 = 0; a9 < 10;){M(L) a8++; }
#define L a9++;

void foo() {
  L1
  L1
  L1
  L1
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120214/c517db7a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: loop-analysis2.patch
Type: text/x-patch
Size: 7204 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120214/c517db7a/attachment.bin>