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.
Index: test/SemaCXX/warn-loop-analysis.cpp
===================================================================
--- test/SemaCXX/warn-loop-analysis.cpp (revision 0)
+++ test/SemaCXX/warn-loop-analysis.cpp (revision 0)
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -fsyntax-only -Wloop-analysis -verify %s
+
+struct S {
+ bool stop() { return false; }
+ bool keep_running;
+};
+
+void by_ref(int &value) { }
+void by_value(int value) { }
+void by_pointer(int *value) {}
+
+void test1() {
+ S s;
+ for (; !s.stop();) {}
+ for (; s.keep_running;) {}
+ for (int i; i < 1; ++i) {}
+ for (int i; i < 1; ) {} // expected-warning {{variables used in loop condition not modified in loop body}}
+ for (int i; i < 1; ) { ++i; }
+ for (int i; i < 1; ) { return; }
+ for (int i; i < 1; ) { break; }
+ for (int i; i < 1; ) { goto exit_loop; }
+exit_loop:
+ for (int i; i < 1; ) { by_ref(i); }
+ for (int i; i < 1; ) { by_value(i); } // expected-warning {{variables used in loop condition not modified in loop body}}
+ for (int i; i < 1; ) { by_pointer(&i); }
+
+ for (int i; i < 1; ++i)
+ for (int j; j < 1; ++j)
+ { }
+ for (int i; i < 1; ++i)
+ for (int j; j < 1; ++i) // expected-warning {{variables used in loop condition not modified in loop body}}
+ { }
+ for (int i; i < 1; ++i)
+ for (int j; i < 1; ++j) // expected-warning {{variables used in loop condition not modified in loop body}}
+ { }
+}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 149887)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -14,6 +14,11 @@
let Component = "Sema" in {
let CategoryName = "Semantic Issue" in {
+// For loop analysis
+def warn_variables_not_in_loop_body : Warning<
+ "variables used in loop condition not modified in loop body">,
+ InGroup<DiagGroup<"loop-analysis">>, DefaultIgnore;
+
// Constant expressions
def err_expr_not_ice : Error<
"expression is not an %select{integer|integral}0 constant expression">;
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp (revision 149887)
+++ lib/Sema/SemaStmt.cpp (working copy)
@@ -19,6 +19,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/CharUnits.h"
#include "clang/AST/DeclObjC.h"
+#include "clang/AST/EvaluatedExprVisitor.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ExprObjC.h"
#include "clang/AST/StmtObjC.h"
@@ -1009,6 +1010,153 @@
return Owned(new (Context) DoStmt(Body, Cond, DoLoc, WhileLoc, CondRParen));
}
+namespace {
+ // This visitor will traverse a conditional statement and store all
+ // the evaluated decls into a vector. Simple is set to true if none
+ // of the excluded constructs are used.
+ class DeclExtractor : public EvaluatedExprVisitor<DeclExtractor> {
+ SmallVectorImpl<ValueDecl*> &Decls;
+ bool Simple;
+public:
+ typedef EvaluatedExprVisitor<DeclExtractor> Inherited;
+
+ DeclExtractor(Sema &S, SmallVectorImpl<ValueDecl*> &Decls) :
+ Inherited(S.Context), Decls(Decls), Simple(true) {}
+
+ bool isSimple() { return Simple; }
+
+ void VisitMemberExpr(MemberExpr *E) {
+ // Don't do analysis on classes.
+ Simple = false;
+ return;
+ }
+
+ void VisitCastExpr(CastExpr *E) {
+ // Don't do analysis on function calls or arrays.
+ if (E->getCastKind() == CK_FunctionToPointerDecay ||
+ E->getCastKind() == CK_ArrayToPointerDecay) {
+ Simple = false;
+ return;
+ }
+
+ Visit(E->getSubExpr());
+ }
+
+ void VisitDeclRefExpr(DeclRefExpr *E) {
+ ValueDecl *VD = E->getDecl();
+ // Dont do analysis on pointers.
+ if (VD->getType()->isAnyPointerType()) {
+ Simple = false;
+ return;
+ }
+
+ // Make sure not to add duplicate Decls.
+ for (SmallVectorImpl<ValueDecl*>::iterator I = Decls.begin(),
+ E = Decls.end();
+ I != E; ++I)
+ if (*I == VD) return;
+
+ Decls.push_back(VD);
+ }
+
+ }; // end class DeclExtractor
+
+ // DeclMatcher checks to see if the decls are used in a non-evauluated
+ // context.
+ class DeclMatcher : public StmtVisitor<DeclMatcher> {
+ SmallVectorImpl<ValueDecl*> &Decls;
+ bool FoundDecl;
+
+public:
+ DeclMatcher(SmallVectorImpl<ValueDecl*> &Decls, Stmt *Statement) :
+ Decls(Decls), FoundDecl(false) {
+ if (!Statement) return;
+ if (Expr *E = dyn_cast<Expr>(Statement)) {
+ VisitExpr(E);
+ return;
+ }
+
+ Visit(Statement);
+ }
+
+ void VisitStmt(Stmt *S) {
+ for (Stmt::child_range C = S->children(); C; ++C)
+ if (*C)
+ this->Visit(*C);
+ }
+
+ void VisitReturnStmt(ReturnStmt *S) {
+ FoundDecl = true;
+ }
+
+ void VisitBreakStmt(BreakStmt *S) {
+ FoundDecl = true;
+ }
+
+ void VisitGotoStmt(GotoStmt *S) {
+ FoundDecl = true;
+ }
+
+ void VisitCastExpr(CastExpr *E) {
+ // Decl is evaluated, not updated. Ignore.
+ if (E->getCastKind() == CK_LValueToRValue)
+ if (isa<DeclRefExpr>(E->getSubExpr()))
+ return;
+
+ VisitExpr(E->getSubExpr());
+ }
+
+ void VisitDeclRefExpr(DeclRefExpr *E) {
+ ValueDecl *VD = E->getDecl();
+ for (SmallVectorImpl<ValueDecl*>::iterator I = Decls.begin(),
+ E = Decls.end();
+ I != E; ++I)
+ if (*I == VD) {
+ FoundDecl = true;
+ return;
+ }
+ }
+
+ bool FoundDeclInUse() { return FoundDecl; }
+
+ }; // end class DeclMatcher
+
+ void CheckForLoopConditionalStatement(Sema &S, Expr *Second,
+ Expr *Third, Stmt *Body) {
+ // Condition is empty
+ if (!Second) return;
+
+ if (S.Diags.getDiagnosticLevel(diag::warn_variables_not_in_loop_body,
+ Second->getLocStart())
+ == DiagnosticsEngine::Ignored)
+ return;
+
+ SmallVector<ValueDecl*, 4> Decls;
+ DeclExtractor DE(S, Decls);
+ DE.Visit(Second);
+
+ // Don't analyze complex conditionals.
+ if (!DE.isSimple()) return;
+
+ // No decls found.
+ if (Decls.size() == 0) return;
+
+ // Don't warn on volatile decls.
+ for (SmallVector<ValueDecl*, 4>::iterator I = Decls.begin(),
+ E = Decls.end();
+ I != E; ++I)
+ if ((*I)->getType().isVolatileQualified()) return;
+
+ if (!DeclMatcher(Decls, Second).FoundDeclInUse() &&
+ !DeclMatcher(Decls, Third).FoundDeclInUse() &&
+ !DeclMatcher(Decls, Body).FoundDeclInUse()) {
+ S.Diag(Second->getLocStart(), diag::warn_variables_not_in_loop_body)
+ << Second->getSourceRange();
+ }
+ }
+
+} // end namespace
+
StmtResult
Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
Stmt *First, FullExprArg second, Decl *secondVar,
@@ -1031,6 +1179,8 @@
}
}
+ CheckForLoopConditionalStatement(*this, second.get(), third.get(), Body);
+
ExprResult SecondResult(second.release());
VarDecl *ConditionVar = 0;
if (secondVar) {
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
