On Tue, Feb 14, 2012 at 3:42 PM, Richard Trieu <[email protected]> wrote:
> On Tue, Feb 7, 2012 at 10:38 AM, Douglas Gregor <[email protected]> wrote: > >> >> On Feb 6, 2012, at 1:00 PM, Eli Friedman wrote: >> >> > On Mon, Feb 6, 2012 at 2:50 PM, Richard Trieu <[email protected]> >> wrote: >> >> On Mon, Feb 6, 2012 at 2:02 PM, Eli Friedman <[email protected]> >> wrote: >> >>> >> >>> On Mon, Feb 6, 2012 at 1:55 PM, Richard Trieu <[email protected]> >> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
