From acdbbc7811c6045a718669a8a0740488edbbbd66 Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Tue, 6 Aug 2013 21:31:54 +0000 Subject: [PATCH] Add a new warning to -Wloop-analysis to detect suspicious increments or decrements inside for loops. Idea for this warning proposed in PR15636: http://llvm.org/bugs/show_bug.cgi?id=15636 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@187817 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 1 + include/clang/Basic/DiagnosticSemaKinds.td | 7 +- lib/Sema/SemaStmt.cpp | 99 +++++++++++++++++++ test/SemaCXX/warn-loop-analysis.cpp | 108 +++++++++++++++++++++ 4 files changed, 214 insertions(+), 1 deletion(-) diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 27752c6e31..d3055748dd 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -163,6 +163,7 @@ def : DiagGroup<"invalid-pch">; def LiteralRange : DiagGroup<"literal-range">; def LocalTypeTemplateArgs : DiagGroup<"local-type-template-args", [CXX98CompatLocalTypeTemplateArgs]>; +def LoopAnalysis : DiagGroup<"loop-analysis">; def MalformedWarningCheck : DiagGroup<"malformed-warning-check">; def Main : DiagGroup<"main">; def MainReturnType : DiagGroup<"main-return-type">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 1ef9c5a059..12ddc6b8a1 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -18,7 +18,12 @@ let CategoryName = "Semantic Issue" in { def warn_variables_not_in_loop_body : Warning< "variable%select{s| %1|s %1 and %2|s %1, %2, and %3|s %1, %2, %3, and %4}0 " "used in loop condition not modified in loop body">, - InGroup>, DefaultIgnore; + InGroup, DefaultIgnore; +def warn_redundant_loop_iteration : Warning< + "variable %0 is %select{decremented|incremented}1 both in the loop header " + "and in the loop body">, + InGroup, DefaultIgnore; +def note_loop_iteration_here : Note<"%select{decremented|incremented}0 here">; def warn_duplicate_enum_values : Warning< "element %0 has been implicitly assigned %1 which another element has " diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 61319d2238..46e350bfa2 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -1418,6 +1418,104 @@ namespace { S.Diag(Ranges.begin()->getBegin(), PDiag); } + // If Statement is an incemement or decrement, return true and sets the + // variables Increment and DRE. + bool ProcessIterationStmt(Sema &S, Stmt* Statement, bool &Increment, + DeclRefExpr *&DRE) { + if (UnaryOperator *UO = dyn_cast(Statement)) { + switch (UO->getOpcode()) { + default: return false; + case UO_PostInc: + case UO_PreInc: + Increment = true; + break; + case UO_PostDec: + case UO_PreDec: + Increment = false; + break; + } + DRE = dyn_cast(UO->getSubExpr()); + return DRE; + } + + if (CXXOperatorCallExpr *Call = dyn_cast(Statement)) { + FunctionDecl *FD = Call->getDirectCallee(); + if (!FD || !FD->isOverloadedOperator()) return false; + switch (FD->getOverloadedOperator()) { + default: return false; + case OO_PlusPlus: + Increment = true; + break; + case OO_MinusMinus: + Increment = false; + break; + } + DRE = dyn_cast(Call->getArg(0)); + return DRE; + } + + return false; + } + + // A visitor to determine if a continue statement is a subexpression. + class ContinueFinder : public EvaluatedExprVisitor { + bool Found; + public: + ContinueFinder(Sema &S, Stmt* Body) : + Inherited(S.Context), + Found(false) { + Visit(Body); + } + + typedef EvaluatedExprVisitor Inherited; + + void VisitContinueStmt(ContinueStmt* E) { + Found = true; + } + + bool ContinueFound() { return Found; } + + }; // end class ContinueFinder + + // Emit a warning when a loop increment/decrement appears twice per loop + // iteration. The conditions which trigger this warning are: + // 1) The last statement in the loop body and the third expression in the + // for loop are both increment or both decrement of the same variable + // 2) No continue statements in the loop body. + void CheckForRedundantIteration(Sema &S, Expr *Third, Stmt *Body) { + // Return when there is nothing to check. + if (!Body || !Third) return; + + if (S.Diags.getDiagnosticLevel(diag::warn_redundant_loop_iteration, + Third->getLocStart()) + == DiagnosticsEngine::Ignored) + return; + + // Get the last statement from the loop body. + CompoundStmt *CS = dyn_cast(Body); + if (!CS || CS->body_empty()) return; + Stmt *LastStmt = CS->body_back(); + if (!LastStmt) return; + + bool LoopIncrement, LastIncrement; + DeclRefExpr *LoopDRE, *LastDRE; + + if (!ProcessIterationStmt(S, Third, LoopIncrement, LoopDRE)) return; + if (!ProcessIterationStmt(S, LastStmt, LastIncrement, LastDRE)) return; + + // Check that the two statements are both increments or both decrements + // on the same varaible. + if (LoopIncrement != LastIncrement || + LoopDRE->getDecl() != LastDRE->getDecl()) return; + + if (ContinueFinder(S, Body).ContinueFound()) return; + + S.Diag(LastDRE->getLocation(), diag::warn_redundant_loop_iteration) + << LastDRE->getDecl() << LastIncrement; + S.Diag(LoopDRE->getLocation(), diag::note_loop_iteration_here) + << LoopIncrement; + } + } // end namespace StmtResult @@ -1444,6 +1542,7 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc, } CheckForLoopConditionalStatement(*this, second.get(), third.get(), Body); + CheckForRedundantIteration(*this, third.get(), Body); ExprResult SecondResult(second.release()); VarDecl *ConditionVar = 0; diff --git a/test/SemaCXX/warn-loop-analysis.cpp b/test/SemaCXX/warn-loop-analysis.cpp index 627bc51d1b..c666c48fc0 100644 --- a/test/SemaCXX/warn-loop-analysis.cpp +++ b/test/SemaCXX/warn-loop-analysis.cpp @@ -152,3 +152,111 @@ void test6() { for (;x6;); for (;y;); } + +void test7() { + int i; + for (;;i++) { // expected-note{{incremented here}} + if (true) test7(); + i++; // expected-warning{{incremented both}} + } + for (;;i++) { // expected-note{{incremented here}} + if (true) break; + ++i; // expected-warning{{incremented both}} + } + for (;;++i) { // expected-note{{incremented here}} + while (true) return; + i++; // expected-warning{{incremented both}} + } + for (;;++i) { // expected-note{{incremented here}} + ++i; // expected-warning{{incremented both}} + } + + for (;;i--) { // expected-note{{decremented here}} + if (true) test7(); + i--; // expected-warning{{decremented both}} + } + for (;;i--) { // expected-note{{decremented here}} + if (true) break; + --i; // expected-warning{{decremented both}} + } + for (;;--i) { // expected-note{{decremented here}} + while (true) return; + i--; // expected-warning{{decremented both}} + } + for (;;--i) { // expected-note{{decremented here}} + --i; // expected-warning{{decremented both}} + } + + // Don't warn when loop is only one statement. + for (;;++i) + i++; + for (;;--i) + --i; + + // Don't warn when loop has continue statement. + for (;;i++) { + if (true) continue; + i++; + } + for (;;i--) { + if (true) continue; + i--; + } +} + +struct iterator { + iterator operator++() { return *this; } + iterator operator++(int) { return *this; } + iterator operator--() { return *this; } + iterator operator--(int) { return *this; } +}; +void test8() { + iterator i; + for (;;i++) { // expected-note{{incremented here}} + if (true) test7(); + i++; // expected-warning{{incremented both}} + } + for (;;i++) { // expected-note{{incremented here}} + if (true) break; + ++i; // expected-warning{{incremented both}} + } + for (;;++i) { // expected-note{{incremented here}} + while (true) return; + i++; // expected-warning{{incremented both}} + } + for (;;++i) { // expected-note{{incremented here}} + ++i; // expected-warning{{incremented both}} + } + + for (;;i--) { // expected-note{{decremented here}} + if (true) test7(); + i--; // expected-warning{{decremented both}} + } + for (;;i--) { // expected-note{{decremented here}} + if (true) break; + --i; // expected-warning{{decremented both}} + } + for (;;--i) { // expected-note{{decremented here}} + while (true) return; + i--; // expected-warning{{decremented both}} + } + for (;;--i) { // expected-note{{decremented here}} + --i; // expected-warning{{decremented both}} + } + + // Don't warn when loop is only one statement. + for (;;++i) + i++; + for (;;--i) + --i; + + // Don't warn when loop has continue statement. + for (;;i++) { + if (true) continue; + i++; + } + for (;;i--) { + if (true) continue; + i--; + } +} -- 2.40.0