<div class="gmail_quote">On Mon, Apr 30, 2012 at 8:01 PM, 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></blockquote><div><br>Awesome!<br>
<br>
Just a nit: it would look better with a comma between 'a' and 'b'.<br><br>-- Matthieu<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ 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>