[cfe-commits] r155835 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaStmt.cpp test/SemaCXX/warn-loop-analysis.cpp
Richard Trieu
rtrieu at google.com
Wed May 2 15:52:37 PDT 2012
On Tue, May 1, 2012 at 2:46 AM, Matthieu Monrocq <matthieu.monrocq at gmail.com
> wrote:
> On Mon, Apr 30, 2012 at 8:01 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> Author: rtrieu
>> Date: Mon Apr 30 13:01:30 2012
>> New Revision: 155835
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=155835&view=rev
>> Log:
>> Add -Wloop-analysis. This warning will fire on for loops which the
>> variables
>> in the loop conditional do not change.
>>
>> Added:
>> cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaStmt.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=155835&r1=155834&r2=155835&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Apr 30
>> 13:01:30 2012
>> @@ -14,6 +14,12 @@
>> let Component = "Sema" in {
>> let CategoryName = "Semantic Issue" in {
>>
>> +// For loop analysis
>> +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;
>> +
>> // Constant expressions
>> def err_expr_not_ice : Error<
>> "expression is not an %select{integer|integral}0 constant expression">;
>>
>> Modified: cfe/trunk/lib/Sema/SemaStmt.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=155835&r1=155834&r2=155835&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaStmt.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Mon Apr 30 13:01:30 2012
>> @@ -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"
>> @@ -28,6 +29,7 @@
>> #include "clang/Basic/TargetInfo.h"
>> #include "llvm/ADT/ArrayRef.h"
>> #include "llvm/ADT/STLExtras.h"
>> +#include "llvm/ADT/SmallPtrSet.h"
>> #include "llvm/ADT/SmallVector.h"
>> using namespace clang;
>> using namespace sema;
>> @@ -1037,6 +1039,218 @@
>> 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> {
>> + llvm::SmallPtrSet<VarDecl*, 8> &Decls;
>> + llvm::SmallVector<SourceRange, 10> &Ranges;
>> + bool Simple;
>> + PartialDiagnostic &PDiag;
>> +public:
>> + typedef EvaluatedExprVisitor<DeclExtractor> Inherited;
>> +
>> + DeclExtractor(Sema &S, llvm::SmallPtrSet<VarDecl*, 8> &Decls,
>> + llvm::SmallVector<SourceRange, 10> &Ranges,
>> + PartialDiagnostic &PDiag) :
>> + Inherited(S.Context),
>> + Decls(Decls),
>> + Ranges(Ranges),
>> + Simple(true),
>> + PDiag(PDiag) {}
>> +
>> + bool isSimple() { return Simple; }
>> +
>> + // Replaces the method in EvaluatedExprVisitor.
>> + void VisitMemberExpr(MemberExpr* E) {
>> + Simple = false;
>> + }
>> +
>> + // Any Stmt not whitelisted will cause the condition to be marked
>> complex.
>> + void VisitStmt(Stmt *S) {
>> + Simple = false;
>> + }
>> +
>> + void VisitBinaryOperator(BinaryOperator *E) {
>> + Visit(E->getLHS());
>> + Visit(E->getRHS());
>> + }
>> +
>> + void VisitCastExpr(CastExpr *E) {
>> + Visit(E->getSubExpr());
>> + }
>> +
>> + void VisitUnaryOperator(UnaryOperator *E) {
>> + // Skip checking conditionals with derefernces.
>> + if (E->getOpcode() == UO_Deref)
>> + Simple = false;
>> + else
>> + Visit(E->getSubExpr());
>> + }
>> +
>> + void VisitConditionalOperator(ConditionalOperator *E) {
>> + Visit(E->getCond());
>> + Visit(E->getTrueExpr());
>> + Visit(E->getFalseExpr());
>> + }
>> +
>> + void VisitParenExpr(ParenExpr *E) {
>> + Visit(E->getSubExpr());
>> + }
>> +
>> + void VisitBinaryConditionalOperator(BinaryConditionalOperator *E) {
>> + Visit(E->getOpaqueValue()->getSourceExpr());
>> + Visit(E->getFalseExpr());
>> + }
>> +
>> + void VisitIntegerLiteral(IntegerLiteral *E) { }
>> + void VisitFloatingLiteral(FloatingLiteral *E) { }
>> + void VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *E) { }
>> + void VisitCharacterLiteral(CharacterLiteral *E) { }
>> + void VisitGNUNullExpr(GNUNullExpr *E) { }
>> + void VisitImaginaryLiteral(ImaginaryLiteral *E) { }
>> +
>> + void VisitDeclRefExpr(DeclRefExpr *E) {
>> + VarDecl *VD = dyn_cast<VarDecl>(E->getDecl());
>> + if (!VD) return;
>> +
>> + Ranges.push_back(E->getSourceRange());
>> +
>> + Decls.insert(VD);
>> + }
>> +
>> + }; // end class DeclExtractor
>> +
>> + // DeclMatcher checks to see if the decls are used in a non-evauluated
>> + // context.
>> + class DeclMatcher : public EvaluatedExprVisitor<DeclMatcher> {
>> + llvm::SmallPtrSet<VarDecl*, 8> &Decls;
>> + bool FoundDecl;
>> + //bool EvalDecl;
>> +
>> +public:
>> + typedef EvaluatedExprVisitor<DeclMatcher> Inherited;
>> +
>> + DeclMatcher(Sema &S, llvm::SmallPtrSet<VarDecl*, 8> &Decls, Stmt
>> *Statement) :
>> + Inherited(S.Context), Decls(Decls), FoundDecl(false) {
>> + if (!Statement) return;
>> +
>> + Visit(Statement);
>> + }
>> +
>> + void VisitReturnStmt(ReturnStmt *S) {
>> + FoundDecl = true;
>> + }
>> +
>> + void VisitBreakStmt(BreakStmt *S) {
>> + FoundDecl = true;
>> + }
>> +
>> + void VisitGotoStmt(GotoStmt *S) {
>> + FoundDecl = true;
>> + }
>> +
>> + void VisitCastExpr(CastExpr *E) {
>> + if (E->getCastKind() == CK_LValueToRValue)
>> + CheckLValueToRValueCast(E->getSubExpr());
>> + else
>> + Visit(E->getSubExpr());
>> + }
>> +
>> + void CheckLValueToRValueCast(Expr *E) {
>> + E = E->IgnoreParenImpCasts();
>> +
>> + if (isa<DeclRefExpr>(E)) {
>> + return;
>> + }
>> +
>> + if (ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E)) {
>> + Visit(CO->getCond());
>> + CheckLValueToRValueCast(CO->getTrueExpr());
>> + CheckLValueToRValueCast(CO->getFalseExpr());
>> + return;
>> + }
>> +
>> + if (BinaryConditionalOperator *BCO =
>> + dyn_cast<BinaryConditionalOperator>(E)) {
>> + CheckLValueToRValueCast(BCO->getOpaqueValue()->getSourceExpr());
>> + CheckLValueToRValueCast(BCO->getFalseExpr());
>> + return;
>> + }
>> +
>> + Visit(E);
>> + }
>> +
>> + void VisitDeclRefExpr(DeclRefExpr *E) {
>> + if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))
>> + if (Decls.count(VD))
>> + FoundDecl = true;
>> + }
>> +
>> + 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;
>> +
>> + PartialDiagnostic PDiag =
>> S.PDiag(diag::warn_variables_not_in_loop_body);
>> + llvm::SmallPtrSet<VarDecl*, 8> Decls;
>> + llvm::SmallVector<SourceRange, 10> Ranges;
>> + DeclExtractor DE(S, Decls, Ranges, PDiag);
>> + 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 (llvm::SmallPtrSet<VarDecl*, 8>::iterator I = Decls.begin(),
>> + E = Decls.end();
>> + I != E; ++I)
>> + if ((*I)->getType().isVolatileQualified()) return;
>> +
>> + if (DeclMatcher(S, Decls, Second).FoundDeclInUse() ||
>> + DeclMatcher(S, Decls, Third).FoundDeclInUse() ||
>> + DeclMatcher(S, Decls, Body).FoundDeclInUse())
>> + return;
>> +
>> + // Load decl names into diagnostic.
>> + if (Decls.size() > 4)
>> + PDiag << 0;
>> + else {
>> + PDiag << Decls.size();
>> + for (llvm::SmallPtrSet<VarDecl*, 8>::iterator I = Decls.begin(),
>> + E = Decls.end();
>> + I != E; ++I)
>> + PDiag << (*I)->getDeclName();
>> + }
>> +
>> + // Load SourceRanges into diagnostic if there is room.
>> + // Otherwise, load the SourceRange of the conditional expression.
>> + if (Ranges.size() <= PartialDiagnostic::MaxArguments)
>> + for (llvm::SmallVector<SourceRange, 10>::iterator I =
>> Ranges.begin(),
>> + E = Ranges.end();
>> + I != E; ++I)
>> + PDiag << *I;
>> + else
>> + PDiag << Second->getSourceRange();
>> +
>> + S.Diag(Ranges.begin()->getBegin(), PDiag);
>> + }
>> +
>> +} // end namespace
>> +
>> StmtResult
>> Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
>> Stmt *First, FullExprArg second, Decl *secondVar,
>> @@ -1059,6 +1273,8 @@
>> }
>> }
>>
>> + CheckForLoopConditionalStatement(*this, second.get(), third.get(),
>> Body);
>> +
>> ExprResult SecondResult(second.release());
>> VarDecl *ConditionVar = 0;
>> if (secondVar) {
>>
>> Added: cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp?rev=155835&view=auto
>>
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp (added)
>> +++ cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp Mon Apr 30 13:01:30 2012
>> @@ -0,0 +1,146 @@
>> +// 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 {{variable 'i' 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 {{variable
>> 'i' 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 {{variable 'j' used in
>> loop condition not modified in loop body}}
>> + { }
>> + for (int i; i < 1; ++i)
>> + for (int j; i < 1; ++j) // expected-warning {{variable 'i' used in
>> loop condition not modified in loop body}}
>> + { }
>> +
>> + for (int *i, *j; i < j; ++i) {}
>> + for (int *i, *j; i < j;) {} // expected-warning {{variables 'i' and
>> 'j' used in loop condition not modified in loop body}}
>> +
>> + // Dereferencing pointers is ignored for now.
>> + for (int *i; *i; ) {}
>> +}
>> +
>> +void test2() {
>> + int i, j, k;
>> + int *ptr;
>> +
>> + // Testing CastExpr
>> + for (; i; ) {} // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; i; ) { i = 5; }
>> +
>> + // Testing BinaryOperator
>> + for (; i < j; ) {} // expected-warning {{variables 'i' and 'j' used in
>> loop condition not modified in loop body}}
>> + for (; i < j; ) { i = 5; }
>> + for (; i < j; ) { j = 5; }
>> +
>> + // Testing IntegerLiteral
>> + for (; i < 5; ) {} // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; i < 5; ) { i = 5; }
>> +
>> + // Testing FloatingLiteral
>> + for (; i < 5.0; ) {} // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; i < 5.0; ) { i = 5; }
>> +
>> + // Testing CharacterLiteral
>> + for (; i == 'a'; ) {} // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; i == 'a'; ) { i = 5; }
>> +
>> + // Testing CXXBoolLiteralExpr
>> + for (; i == true; ) {} // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; i == true; ) { i = 5; }
>> +
>> + // Testing GNUNullExpr
>> + for (; ptr == __null; ) {} // expected-warning {{variable 'ptr' used
>> in loop condition not modified in loop body}}
>> + for (; ptr == __null; ) { ptr = &i; }
>> +
>> + // Testing UnaryOperator
>> + for (; -i > 5; ) {} // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; -i > 5; ) { ++i; }
>> +
>> + // Testing ImaginaryLiteral
>> + for (; i != 3i; ) {} // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; i != 3i; ) { ++i; }
>> +
>> + // Testing ConditionalOperator
>> + for (; i ? j : k; ) {} // expected-warning {{variables 'i' 'j' and 'k'
>> used in loop condition not modified in loop body}}
>> + for (; i ? j : k; ) { ++i; }
>> + for (; i ? j : k; ) { ++j; }
>> + for (; i ? j : k; ) { ++k; }
>> + for (; i; ) { j = i ? i : i; } // expected-warning {{variable 'i'
>> used in loop condition not modified in loop body}}
>> + for (; i; ) { j = (i = 1) ? i : i; }
>> + for (; i; ) { j = i ? i : ++i; }
>> +
>> + // Testing BinaryConditionalOperator
>> + for (; i ?: j; ) {} // expected-warning {{variables 'i' and 'j' used
>> in loop condition not modified in loop body}}
>> + for (; i ?: j; ) { ++i; }
>> + for (; i ?: j; ) { ++j; }
>> + for (; i; ) { j = i ?: i; } // expected-warning {{variable 'i' used
>> in loop condition not modified in loop body}}
>> +
>> + // Testing ParenExpr
>> + for (; (i); ) { } // expected-warning {{variable 'i' used in loop
>> condition not modified in loop body}}
>> + for (; (i); ) { ++i; }
>> +
>> + // Testing non-evaluated variables
>> + for (; i < sizeof(j); ) { } // expected-warning {{variable 'i' used
>> in loop condition not modified in loop body}}
>> + for (; i < sizeof(j); ) { ++j; } // expected-warning {{variable 'i'
>> used in loop condition not modified in loop body}}
>> + for (; i < sizeof(j); ) { ++i; }
>> +}
>> +
>> +// False positive and how to silence.
>> +void test3() {
>> + int x;
>> + int *ptr = &x;
>> + for (;x<5;) { *ptr = 6; } // expected-warning {{variable 'x' used in
>> loop condition not modified in loop body}}
>> +
>> + for (;x<5;) {
>> + *ptr = 6;
>> + (void)x;
>> + }
>> +}
>> +
>> +// Check ordering and printing of variables. Max variables is currently
>> 4.
>> +void test4() {
>> + int a, b, c, d, e, f;
>> + for (; a;); // expected-warning {{variable 'a' used in loop condition
>> not modified in loop body}}
>> + for (; a + b;); // expected-warning {{variables 'a' and 'b' used in
>> loop condition not modified in loop body}}
>> + for (; a + b + c;); // expected-warning {{variables 'a' 'b' and 'c'
>> used in loop condition not modified in loop body}}
>>
>
> Awesome!
>
> Just a nit: it would look better with a comma between 'a' and 'b'.
>
Now with more commas. r156033.
>
> -- Matthieu
>
>
>> + for (; a + b + c + d;); // expected-warning {{variables 'a' 'b' 'c'
>> and 'd' used in loop condition not modified in loop body}}
>> + for (; a + b + c + d + e;); // expected-warning {{variables used in
>> loop condition not modified in loop body}}
>> + for (; a + b + c + d + e + f;); // expected-warning {{variables used
>> in loop condition not modified in loop body}}
>> + for (; a + c + d + b;); // expected-warning {{variables 'a' 'c' 'd'
>> and 'b' used in loop condition not modified in loop body}}
>> + for (; d + c + b + a;); // expected-warning {{variables 'd' 'c' 'b'
>> and 'a' used in loop condition not modified in loop body}}
>> +}
>> +
>> +// Ensure that the warning doesn't fail when lots of variables are used
>> +// in the conditional.
>> +void test5() {
>> + for (int a; a+a+a+a+a+a+a+a+a+a;); // \
>> + // expected-warning {{variable 'a' used in loop condition not
>> modified in loop body}}
>> + for (int a; a+a+a+a+a+a+a+a+a+a+a;); // \
>> + // expected-warning {{variable 'a' used in loop condition not
>> modified in loop body}}
>> + for (int a; a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a;); // \
>> + // expected-warning {{variable 'a' used in loop condition not
>> modified in loop body}}
>> + 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;);//\
>> + // expected-warning {{variable 'a' used in loop condition not
>> modified in loop body}}
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120502/cbfb2da8/attachment.html>
More information about the cfe-commits
mailing list