On Tue, Feb 14, 2012 at 6:37 PM, Richard Trieu <[email protected]> wrote:
> 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 > > > Ping.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
