Hi Richard,<div><br></div><div>if you're looking for data if this should be on by default: This has 0 false positives in all of chromium (including tests, all dependencies) and found 0 bugs.</div><div><br></div><div>(I did put in a true positive ("for (int i; i < 5;) {}") somewhere and verified that that got detected, to make sure I enabled the warning correctly. My first attempt to introduce a true positive, "int i; while (i > 0) {}" didn't fire, I suppose while loops aren't handled intentionally?)</div>
<div><br></div><div>Nico<br><br><div class="gmail_quote">On Mon, Apr 30, 2012 at 11:01 AM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: rtrieu<br>
Date: Mon Apr 30 13:01:30 2012<br>
New Revision: 155835<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=155835&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=155835&view=rev</a><br>
Log:<br>
Add -Wloop-analysis.  This warning will fire on for loops which the variables<br>
in the loop conditional do not change.<br>
<br>
Added:<br>
    cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp<br>
Modified:<br>
    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
    cfe/trunk/lib/Sema/SemaStmt.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=155835&r1=155834&r2=155835&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=155835&r1=155834&r2=155835&view=diff</a><br>

==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Apr 30 13:01:30 2012<br>
@@ -14,6 +14,12 @@<br>
 let Component = "Sema" in {<br>
 let CategoryName = "Semantic Issue" in {<br>
<br>
+// For loop analysis<br>
+def warn_variables_not_in_loop_body : Warning<<br>
+  "variable%select{s| %1|s %1 and %2|s %1 %2 and %3|s %1 %2 %3 and %4}0 "<br>
+  "used in loop condition not modified in loop body">,<br>
+  InGroup<DiagGroup<"loop-analysis">>, DefaultIgnore;<br>
+<br>
 // Constant expressions<br>
 def err_expr_not_ice : Error<<br>
   "expression is not an %select{integer|integral}0 constant expression">;<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaStmt.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=155835&r1=155834&r2=155835&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=155835&r1=155834&r2=155835&view=diff</a><br>

==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaStmt.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaStmt.cpp Mon Apr 30 13:01:30 2012<br>
@@ -19,6 +19,7 @@<br>
 #include "clang/AST/ASTContext.h"<br>
 #include "clang/AST/CharUnits.h"<br>
 #include "clang/AST/DeclObjC.h"<br>
+#include "clang/AST/EvaluatedExprVisitor.h"<br>
 #include "clang/AST/ExprCXX.h"<br>
 #include "clang/AST/ExprObjC.h"<br>
 #include "clang/AST/StmtObjC.h"<br>
@@ -28,6 +29,7 @@<br>
 #include "clang/Basic/TargetInfo.h"<br>
 #include "llvm/ADT/ArrayRef.h"<br>
 #include "llvm/ADT/STLExtras.h"<br>
+#include "llvm/ADT/SmallPtrSet.h"<br>
 #include "llvm/ADT/SmallVector.h"<br>
 using namespace clang;<br>
 using namespace sema;<br>
@@ -1037,6 +1039,218 @@<br>
   return Owned(new (Context) DoStmt(Body, Cond, DoLoc, WhileLoc, CondRParen));<br>
 }<br>
<br>
+namespace {<br>
+  // This visitor will traverse a conditional statement and store all<br>
+  // the evaluated decls into a vector.  Simple is set to true if none<br>
+  // of the excluded constructs are used.<br>
+  class DeclExtractor : public EvaluatedExprVisitor<DeclExtractor> {<br>
+    llvm::SmallPtrSet<VarDecl*, 8> &Decls;<br>
+    llvm::SmallVector<SourceRange, 10> &Ranges;<br>
+    bool Simple;<br>
+    PartialDiagnostic &PDiag;<br>
+public:<br>
+  typedef EvaluatedExprVisitor<DeclExtractor> Inherited;<br>
+<br>
+  DeclExtractor(Sema &S, llvm::SmallPtrSet<VarDecl*, 8> &Decls,<br>
+                llvm::SmallVector<SourceRange, 10> &Ranges,<br>
+                PartialDiagnostic &PDiag) :<br>
+      Inherited(S.Context),<br>
+      Decls(Decls),<br>
+      Ranges(Ranges),<br>
+      Simple(true),<br>
+      PDiag(PDiag) {}<br>
+<br>
+  bool isSimple() { return Simple; }<br>
+<br>
+  // Replaces the method in EvaluatedExprVisitor.<br>
+  void VisitMemberExpr(MemberExpr* E) {<br>
+    Simple = false;<br>
+  }<br>
+<br>
+  // Any Stmt not whitelisted will cause the condition to be marked complex.<br>
+  void VisitStmt(Stmt *S) {<br>
+    Simple = false;<br>
+  }<br>
+<br>
+  void VisitBinaryOperator(BinaryOperator *E) {<br>
+    Visit(E->getLHS());<br>
+    Visit(E->getRHS());<br>
+  }<br>
+<br>
+  void VisitCastExpr(CastExpr *E) {<br>
+    Visit(E->getSubExpr());<br>
+  }<br>
+<br>
+  void VisitUnaryOperator(UnaryOperator *E) {<br>
+    // Skip checking conditionals with derefernces.<br>
+    if (E->getOpcode() == UO_Deref)<br>
+      Simple = false;<br>
+    else<br>
+      Visit(E->getSubExpr());<br>
+  }<br>
+<br>
+  void VisitConditionalOperator(ConditionalOperator *E) {<br>
+    Visit(E->getCond());<br>
+    Visit(E->getTrueExpr());<br>
+    Visit(E->getFalseExpr());<br>
+  }<br>
+<br>
+  void VisitParenExpr(ParenExpr *E) {<br>
+    Visit(E->getSubExpr());<br>
+  }<br>
+<br>
+  void VisitBinaryConditionalOperator(BinaryConditionalOperator *E) {<br>
+    Visit(E->getOpaqueValue()->getSourceExpr());<br>
+    Visit(E->getFalseExpr());<br>
+  }<br>
+<br>
+  void VisitIntegerLiteral(IntegerLiteral *E) { }<br>
+  void VisitFloatingLiteral(FloatingLiteral *E) { }<br>
+  void VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *E) { }<br>
+  void VisitCharacterLiteral(CharacterLiteral *E) { }<br>
+  void VisitGNUNullExpr(GNUNullExpr *E) { }<br>
+  void VisitImaginaryLiteral(ImaginaryLiteral *E) { }<br>
+<br>
+  void VisitDeclRefExpr(DeclRefExpr *E) {<br>
+    VarDecl *VD = dyn_cast<VarDecl>(E->getDecl());<br>
+    if (!VD) return;<br>
+<br>
+    Ranges.push_back(E->getSourceRange());<br>
+<br>
+    Decls.insert(VD);<br>
+  }<br>
+<br>
+  }; // end class DeclExtractor<br>
+<br>
+  // DeclMatcher checks to see if the decls are used in a non-evauluated<br>
+  // context.<br>
+  class DeclMatcher : public EvaluatedExprVisitor<DeclMatcher> {<br>
+    llvm::SmallPtrSet<VarDecl*, 8> &Decls;<br>
+    bool FoundDecl;<br>
+    //bool EvalDecl;<br>
+<br>
+public:<br>
+  typedef EvaluatedExprVisitor<DeclMatcher> Inherited;<br>
+<br>
+  DeclMatcher(Sema &S, llvm::SmallPtrSet<VarDecl*, 8> &Decls, Stmt *Statement) :<br>
+      Inherited(S.Context), Decls(Decls), FoundDecl(false) {<br>
+    if (!Statement) return;<br>
+<br>
+    Visit(Statement);<br>
+  }<br>
+<br>
+  void VisitReturnStmt(ReturnStmt *S) {<br>
+    FoundDecl = true;<br>
+  }<br>
+<br>
+  void VisitBreakStmt(BreakStmt *S) {<br>
+    FoundDecl = true;<br>
+  }<br>
+<br>
+  void VisitGotoStmt(GotoStmt *S) {<br>
+    FoundDecl = true;<br>
+  }<br>
+<br>
+  void VisitCastExpr(CastExpr *E) {<br>
+    if (E->getCastKind() == CK_LValueToRValue)<br>
+      CheckLValueToRValueCast(E->getSubExpr());<br>
+    else<br>
+      Visit(E->getSubExpr());<br>
+  }<br>
+<br>
+  void CheckLValueToRValueCast(Expr *E) {<br>
+    E = E->IgnoreParenImpCasts();<br>
+<br>
+    if (isa<DeclRefExpr>(E)) {<br>
+      return;<br>
+    }<br>
+<br>
+    if (ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E)) {<br>
+      Visit(CO->getCond());<br>
+      CheckLValueToRValueCast(CO->getTrueExpr());<br>
+      CheckLValueToRValueCast(CO->getFalseExpr());<br>
+      return;<br>
+    }<br>
+<br>
+    if (BinaryConditionalOperator *BCO =<br>
+            dyn_cast<BinaryConditionalOperator>(E)) {<br>
+      CheckLValueToRValueCast(BCO->getOpaqueValue()->getSourceExpr());<br>
+      CheckLValueToRValueCast(BCO->getFalseExpr());<br>
+      return;<br>
+    }<br>
+<br>
+    Visit(E);<br>
+  }<br>
+<br>
+  void VisitDeclRefExpr(DeclRefExpr *E) {<br>
+    if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))<br>
+      if (Decls.count(VD))<br>
+        FoundDecl = true;<br>
+  }<br>
+<br>
+  bool FoundDeclInUse() { return FoundDecl; }<br>
+<br>
+  };  // end class DeclMatcher<br>
+<br>
+  void CheckForLoopConditionalStatement(Sema &S, Expr *Second,<br>
+                                        Expr *Third, Stmt *Body) {<br>
+    // Condition is empty<br>
+    if (!Second) return;<br>
+<br>
+    if (S.Diags.getDiagnosticLevel(diag::warn_variables_not_in_loop_body,<br>
+                                   Second->getLocStart())<br>
+        == DiagnosticsEngine::Ignored)<br>
+      return;<br>
+<br>
+    PartialDiagnostic PDiag = S.PDiag(diag::warn_variables_not_in_loop_body);<br>
+    llvm::SmallPtrSet<VarDecl*, 8> Decls;<br>
+    llvm::SmallVector<SourceRange, 10> Ranges;<br>
+    DeclExtractor DE(S, Decls, Ranges, PDiag);<br>
+    DE.Visit(Second);<br>
+<br>
+    // Don't analyze complex conditionals.<br>
+    if (!DE.isSimple()) return;<br>
+<br>
+    // No decls found.<br>
+    if (Decls.size() == 0) return;<br>
+<br>
+    // Don't warn on volatile decls.<br>
+    for (llvm::SmallPtrSet<VarDecl*, 8>::iterator I = Decls.begin(),<br>
+                                                  E = Decls.end();<br>
+         I != E; ++I)<br>
+      if ((*I)->getType().isVolatileQualified()) return;<br>
+<br>
+    if (DeclMatcher(S, Decls, Second).FoundDeclInUse() ||<br>
+        DeclMatcher(S, Decls, Third).FoundDeclInUse() ||<br>
+        DeclMatcher(S, Decls, Body).FoundDeclInUse())<br>
+      return;<br>
+<br>
+    // Load decl names into diagnostic.<br>
+    if (Decls.size() > 4)<br>
+      PDiag << 0;<br>
+    else {<br>
+      PDiag << Decls.size();<br>
+      for (llvm::SmallPtrSet<VarDecl*, 8>::iterator I = Decls.begin(),<br>
+                                                    E = Decls.end();<br>
+           I != E; ++I)<br>
+        PDiag << (*I)->getDeclName();<br>
+    }<br>
+<br>
+    // Load SourceRanges into diagnostic if there is room.<br>
+    // Otherwise, load the SourceRange of the conditional expression.<br>
+    if (Ranges.size() <= PartialDiagnostic::MaxArguments)<br>
+      for (llvm::SmallVector<SourceRange, 10>::iterator I = Ranges.begin(),<br>
+                                                        E = Ranges.end();<br>
+           I != E; ++I)<br>
+        PDiag << *I;<br>
+    else<br>
+      PDiag << Second->getSourceRange();<br>
+<br>
+    S.Diag(Ranges.begin()->getBegin(), PDiag);<br>
+  }<br>
+<br>
+} // end namespace<br>
+<br>
 StmtResult<br>
 Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,<br>
                    Stmt *First, FullExprArg second, Decl *secondVar,<br>
@@ -1059,6 +1273,8 @@<br>
     }<br>
   }<br>
<br>
+  CheckForLoopConditionalStatement(*this, second.get(), third.get(), Body);<br>
+<br>
   ExprResult SecondResult(second.release());<br>
   VarDecl *ConditionVar = 0;<br>
   if (secondVar) {<br>
<br>
Added: cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp?rev=155835&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp?rev=155835&view=auto</a><br>

==============================================================================<br>
--- cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp (added)<br>
+++ cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp Mon Apr 30 13:01:30 2012<br>
@@ -0,0 +1,146 @@<br>
+// RUN: %clang_cc1 -fsyntax-only -Wloop-analysis -verify %s<br>
+<br>
+struct S {<br>
+  bool stop() { return false; }<br>
+  bool keep_running;<br>
+};<br>
+<br>
+void by_ref(int &value) { }<br>
+void by_value(int value) { }<br>
+void by_pointer(int *value) {}<br>
+<br>
+void test1() {<br>
+  S s;<br>
+  for (; !s.stop();) {}<br>
+  for (; s.keep_running;) {}<br>
+  for (int i; i < 1; ++i) {}<br>
+  for (int i; i < 1; ) {}  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (int i; i < 1; ) { ++i; }<br>
+  for (int i; i < 1; ) { return; }<br>
+  for (int i; i < 1; ) { break; }<br>
+  for (int i; i < 1; ) { goto exit_loop; }<br>
+exit_loop:<br>
+  for (int i; i < 1; ) { by_ref(i); }<br>
+  for (int i; i < 1; ) { by_value(i); }  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (int i; i < 1; ) { by_pointer(&i); }<br>
+<br>
+  for (int i; i < 1; ++i)<br>
+    for (int j; j < 1; ++j)<br>
+      { }<br>
+  for (int i; i < 1; ++i)<br>
+    for (int j; j < 1; ++i)  // expected-warning {{variable 'j' used in loop condition not modified in loop body}}<br>
+      { }<br>
+  for (int i; i < 1; ++i)<br>
+    for (int j; i < 1; ++j)  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+      { }<br>
+<br>
+  for (int *i, *j; i < j; ++i) {}<br>
+  for (int *i, *j; i < j;) {}  // expected-warning {{variables 'i' and 'j' used in loop condition not modified in loop body}}<br>
+<br>
+  // Dereferencing pointers is ignored for now.<br>
+  for (int *i; *i; ) {}<br>
+}<br>
+<br>
+void test2() {<br>
+  int i, j, k;<br>
+  int *ptr;<br>
+<br>
+  // Testing CastExpr<br>
+  for (; i; ) {} // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i; ) { i = 5; }<br>
+<br>
+  // Testing BinaryOperator<br>
+  for (; i < j; ) {} // expected-warning {{variables 'i' and 'j' used in loop condition not modified in loop body}}<br>
+  for (; i < j; ) { i = 5; }<br>
+  for (; i < j; ) { j = 5; }<br>
+<br>
+  // Testing IntegerLiteral<br>
+  for (; i < 5; ) {} // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i < 5; ) { i = 5; }<br>
+<br>
+  // Testing FloatingLiteral<br>
+  for (; i < 5.0; ) {} // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i < 5.0; ) { i = 5; }<br>
+<br>
+  // Testing CharacterLiteral<br>
+  for (; i == 'a'; ) {} // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i == 'a'; ) { i = 5; }<br>
+<br>
+  // Testing CXXBoolLiteralExpr<br>
+  for (; i == true; ) {} // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i == true; ) { i = 5; }<br>
+<br>
+  // Testing GNUNullExpr<br>
+  for (; ptr == __null; ) {} // expected-warning {{variable 'ptr' used in loop condition not modified in loop body}}<br>
+  for (; ptr == __null; ) { ptr = &i; }<br>
+<br>
+  // Testing UnaryOperator<br>
+  for (; -i > 5; ) {} // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; -i > 5; ) { ++i; }<br>
+<br>
+  // Testing ImaginaryLiteral<br>
+  for (; i != 3i; ) {} // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i != 3i; ) { ++i; }<br>
+<br>
+  // Testing ConditionalOperator<br>
+  for (; i ? j : k; ) {} // expected-warning {{variables 'i' 'j' and 'k' used in loop condition not modified in loop body}}<br>
+  for (; i ? j : k; ) { ++i; }<br>
+  for (; i ? j : k; ) { ++j; }<br>
+  for (; i ? j : k; ) { ++k; }<br>
+  for (; i; ) { j = i ? i : i; }  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i; ) { j = (i = 1) ? i : i; }<br>
+  for (; i; ) { j = i ? i : ++i; }<br>
+<br>
+  // Testing BinaryConditionalOperator<br>
+  for (; i ?: j; ) {} // expected-warning {{variables 'i' and 'j' used in loop condition not modified in loop body}}<br>
+  for (; i ?: j; ) { ++i; }<br>
+  for (; i ?: j; ) { ++j; }<br>
+  for (; i; ) { j = i ?: i; }  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+<br>
+  // Testing ParenExpr<br>
+  for (; (i); ) { }  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; (i); ) { ++i; }<br>
+<br>
+  // Testing non-evaluated variables<br>
+  for (; i < sizeof(j); ) { }  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i < sizeof(j); ) { ++j; }  // expected-warning {{variable 'i' used in loop condition not modified in loop body}}<br>
+  for (; i < sizeof(j); ) { ++i; }<br>
+}<br>
+<br>
+// False positive and how to silence.<br>
+void test3() {<br>
+  int x;<br>
+  int *ptr = &x;<br>
+  for (;x<5;) { *ptr = 6; }  // expected-warning {{variable 'x' used in loop condition not modified in loop body}}<br>
+<br>
+  for (;x<5;) {<br>
+    *ptr = 6;<br>
+    (void)x;<br>
+  }<br>
+}<br>
+<br>
+// Check ordering and printing of variables.  Max variables is currently 4.<br>
+void test4() {<br>
+  int a, b, c, d, e, f;<br>
+  for (; a;);  // expected-warning {{variable 'a' used in loop condition not modified in loop body}}<br>
+  for (; a + b;);  // expected-warning {{variables 'a' and 'b' used in loop condition not modified in loop body}}<br>
+  for (; a + b + c;);  // expected-warning {{variables 'a' 'b' and 'c' used in loop condition not modified in loop body}}<br>
+  for (; a + b + c + d;);  // expected-warning {{variables 'a' 'b' 'c' and 'd' used in loop condition not modified in loop body}}<br>
+  for (; a + b + c + d + e;);  // expected-warning {{variables used in loop condition not modified in loop body}}<br>
+  for (; a + b + c + d + e + f;);  // expected-warning {{variables used in loop condition not modified in loop body}}<br>
+  for (; a + c + d + b;);  // expected-warning {{variables 'a' 'c' 'd' and 'b' used in loop condition not modified in loop body}}<br>
+  for (; d + c + b + a;);  // expected-warning {{variables 'd' 'c' 'b' and 'a' used in loop condition not modified in loop body}}<br>
+}<br>
+<br>
+// Ensure that the warning doesn't fail when lots of variables are used<br>
+// in the conditional.<br>
+void test5() {<br>
+  for (int a; a+a+a+a+a+a+a+a+a+a;); // \<br>
+   // expected-warning {{variable 'a' used in loop condition not modified in loop body}}<br>
+  for (int a; a+a+a+a+a+a+a+a+a+a+a;); // \<br>
+   // expected-warning {{variable 'a' used in loop condition not modified in loop body}}<br>
+  for (int a; a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a;);  // \<br>
+   // expected-warning {{variable 'a' used in loop condition not modified in loop body}}<br>
+  for (int a; a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a;);//\<br>
+   // expected-warning {{variable 'a' used in loop condition not modified in loop body}}<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>