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

llvm-svn: 187817

clang/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">;

clang/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<DiagGroup<"loop-analysis">>, DefaultIgnore;
+  InGroup<LoopAnalysis>, 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<LoopAnalysis>, 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 "

clang/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<UnaryOperator>(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<DeclRefExpr>(UO->getSubExpr());
+      return DRE;
+    }
+
+    if (CXXOperatorCallExpr *Call = dyn_cast<CXXOperatorCallExpr>(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<DeclRefExpr>(Call->getArg(0));
+      return DRE;
+    }
+
+    return false;
+  }
+
+  // A visitor to determine if a continue statement is a subexpression.
+  class ContinueFinder : public EvaluatedExprVisitor<ContinueFinder> {
+    bool Found;
+  public:
+    ContinueFinder(Sema &S, Stmt* Body) :
+        Inherited(S.Context),
+        Found(false) {
+      Visit(Body);
+    }
+
+    typedef EvaluatedExprVisitor<ContinueFinder> 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<CompoundStmt>(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;

clang/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--;
+  }
+}