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

[Richard Trieu]

On Tue, Feb 14, 2012 at 3:42 PM, Richard Trieu <rtrieu at google.com> wrote:

> 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
> }
>

Some data from real code.  Took Clang source and preprocessed it into 300k
lines of code (if that seems high, I accidentally copied the files twice).
 Ran the patched Clang across the files with either -Wloop-analysis set or
omitted.  -fsyntax-only was also used.

Without -Wloop-analysis,
Min time = 24.99s
Max time = 25.63s
Average = 25.218s

With -Wloop-analysis,
Min time = 25s
Max time = 25.5s
Average = 25.214s
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120214/1af430e3/attachment.html>