r201701 - [analyzer] Extend IdenticalExprChecker to check the two branches of an if.
Jordan Rose
jordan_rose at apple.com
Wed Feb 19 09:44:11 PST 2014
Author: jrose
Date: Wed Feb 19 11:44:11 2014
New Revision: 201701
URL: http://llvm.org/viewvc/llvm-project?rev=201701&view=rev
Log:
[analyzer] Extend IdenticalExprChecker to check the two branches of an if.
This extends the checks for identical expressions to handle identical
statements, and compares the consequent and alternative ("then" and "else")
branches of an if-statement to see if they are identical, treating a single
statement surrounded by braces as equivalent to one without braces.
This does /not/ check subsequent branches in an if/else chain, let alone
branches that are not consecutive. This may improve in a future patch, but
it would certainly take more work.
Patch by Daniel Fahlgren!
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
cfe/trunk/test/Analysis/identical-expressions.cpp
cfe/trunk/test/Analysis/misc-ps-region-store.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp?rev=201701&r1=201700&r2=201701&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp Wed Feb 19 11:44:11 2014
@@ -26,8 +26,8 @@
using namespace clang;
using namespace ento;
-static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
- const Expr *Expr2, bool IgnoreSideEffects = false);
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+ const Stmt *Stmt2, bool IgnoreSideEffects = false);
//===----------------------------------------------------------------------===//
// FindIdenticalExprVisitor - Identify nodes using identical expressions.
//===----------------------------------------------------------------------===//
@@ -41,8 +41,10 @@ public:
AnalysisDeclContext *A)
: BR(B), Checker(Checker), AC(A) {}
// FindIdenticalExprVisitor only visits nodes
- // that are binary operators or conditional operators.
+ // that are binary operators, if statements or
+ // conditional operators.
bool VisitBinaryOperator(const BinaryOperator *B);
+ bool VisitIfStmt(const IfStmt *I);
bool VisitConditionalOperator(const ConditionalOperator *C);
private:
@@ -52,6 +54,39 @@ private:
};
} // end anonymous namespace
+bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) {
+ const Stmt *Stmt1 = I->getThen();
+ const Stmt *Stmt2 = I->getElse();
+
+ if (!Stmt1 || !Stmt2)
+ return true;
+
+ // Special handling for code like:
+ //
+ // if (b) {
+ // i = 1;
+ // } else
+ // i = 1;
+ if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt1)) {
+ if (CompStmt->size() == 1)
+ Stmt1 = CompStmt->body_back();
+ }
+ if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt2)) {
+ if (CompStmt->size() == 1)
+ Stmt2 = CompStmt->body_back();
+ }
+
+ if (isIdenticalStmt(AC->getASTContext(), Stmt1, Stmt2, true)) {
+ PathDiagnosticLocation ELoc =
+ PathDiagnosticLocation::createBegin(I, BR.getSourceManager(), AC);
+ BR.EmitBasicReport(AC->getDecl(), Checker,
+ "Identical branches",
+ categories::LogicError,
+ "true and false branches are identical", ELoc);
+ }
+ return true;
+}
+
bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
BinaryOperator::Opcode Op = B->getOpcode();
if (!BinaryOperator::isComparisonOp(Op))
@@ -107,7 +142,7 @@ bool FindIdenticalExprVisitor::VisitBina
// No special case with floating-point representation, report as usual.
}
- if (isIdenticalExpr(AC->getASTContext(), B->getLHS(), B->getRHS())) {
+ if (isIdenticalStmt(AC->getASTContext(), B->getLHS(), B->getRHS())) {
PathDiagnosticLocation ELoc =
PathDiagnosticLocation::createOperatorLoc(B, BR.getSourceManager());
StringRef Message;
@@ -131,7 +166,7 @@ bool FindIdenticalExprVisitor::VisitCond
// Check if expressions in conditional expression are identical
// from a symbolic point of view.
- if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),
+ if (isIdenticalStmt(AC->getASTContext(), C->getTrueExpr(),
C->getFalseExpr(), true)) {
PathDiagnosticLocation ELoc =
PathDiagnosticLocation::createConditionalColonLoc(
@@ -153,7 +188,7 @@ bool FindIdenticalExprVisitor::VisitCond
return true;
}
-/// \brief Determines whether two expression trees are identical regarding
+/// \brief Determines whether two statement trees are identical regarding
/// operators and symbols.
///
/// Exceptions: expressions containing macros or functions with possible side
@@ -161,81 +196,183 @@ bool FindIdenticalExprVisitor::VisitCond
/// Limitations: (t + u) and (u + t) are not considered identical.
/// t*(u + t) and t*u + t*t are not considered identical.
///
-static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
- const Expr *Expr2, bool IgnoreSideEffects) {
- // If Expr1 & Expr2 are of different class then they are not
- // identical expression.
- if (Expr1->getStmtClass() != Expr2->getStmtClass())
- return false;
- // If Expr1 has side effects then don't warn even if expressions
- // are identical.
- if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
- return false;
- // If either expression comes from a macro then don't warn even if
- // the expressions are identical.
- if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
- return false;
- // If all children of two expressions are identical, return true.
- Expr::const_child_iterator I1 = Expr1->child_begin();
- Expr::const_child_iterator I2 = Expr2->child_begin();
- while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
- const Expr *Child1 = dyn_cast<Expr>(*I1);
- const Expr *Child2 = dyn_cast<Expr>(*I2);
- if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,
- IgnoreSideEffects))
- return false;
- ++I1;
- ++I2;
- }
- // If there are different number of children in the expressions, return false.
- // (TODO: check if this is a redundant condition.)
- if (I1 != Expr1->child_end())
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+ const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+ if (!Stmt1 && !Stmt2)
+ return true;
return false;
- if (I2 != Expr2->child_end())
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
return false;
- switch (Expr1->getStmtClass()) {
+ const Expr *Expr1 = dyn_cast<Expr>(Stmt1);
+ const Expr *Expr2 = dyn_cast<Expr>(Stmt2);
+
+ if (Expr1 && Expr2) {
+ // If Stmt1 has side effects then don't warn even if expressions
+ // are identical.
+ if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+ // If either expression comes from a macro then don't warn even if
+ // the expressions are identical.
+ if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+ // If all children of two expressions are identical, return true.
+ Expr::const_child_iterator I1 = Expr1->child_begin();
+ Expr::const_child_iterator I2 = Expr2->child_begin();
+ while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+ return false;
+ ++I1;
+ ++I2;
+ }
+ // If there are different number of children in the statements, return
+ // false.
+ if (I1 != Expr1->child_end())
+ return false;
+ if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
default:
return false;
case Stmt::CallExprClass:
case Stmt::ArraySubscriptExprClass:
- case Stmt::CStyleCastExprClass:
case Stmt::ImplicitCastExprClass:
case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
return true;
+ case Stmt::CStyleCastExprClass: {
+ const CStyleCastExpr* CastExpr1 = cast<CStyleCastExpr>(Stmt1);
+ const CStyleCastExpr* CastExpr2 = cast<CStyleCastExpr>(Stmt2);
+
+ return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+ const ReturnStmt *ReturnStmt1 = cast<ReturnStmt>(Stmt1);
+ const ReturnStmt *ReturnStmt2 = cast<ReturnStmt>(Stmt2);
+
+ return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+ const ForStmt *ForStmt1 = cast<ForStmt>(Stmt1);
+ const ForStmt *ForStmt2 = cast<ForStmt>(Stmt2);
+
+ if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
+ IgnoreSideEffects))
+ return false;
+ return true;
+ }
+ case Stmt::DoStmtClass: {
+ const DoStmt *DStmt1 = cast<DoStmt>(Stmt1);
+ const DoStmt *DStmt2 = cast<DoStmt>(Stmt2);
+
+ if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
+ IgnoreSideEffects))
+ return false;
+ return true;
+ }
+ case Stmt::WhileStmtClass: {
+ const WhileStmt *WStmt1 = cast<WhileStmt>(Stmt1);
+ const WhileStmt *WStmt2 = cast<WhileStmt>(Stmt2);
+
+ return isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
+ IgnoreSideEffects);
+ }
+ case Stmt::IfStmtClass: {
+ const IfStmt *IStmt1 = cast<IfStmt>(Stmt1);
+ const IfStmt *IStmt2 = cast<IfStmt>(Stmt2);
+
+ if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
+ IgnoreSideEffects))
+ return false;
+ if (!isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
+ IgnoreSideEffects))
+ return false;
+ return true;
+ }
+ case Stmt::CompoundStmtClass: {
+ const CompoundStmt *CompStmt1 = cast<CompoundStmt>(Stmt1);
+ const CompoundStmt *CompStmt2 = cast<CompoundStmt>(Stmt2);
+
+ if (CompStmt1->size() != CompStmt2->size())
+ return false;
+
+ CompoundStmt::const_body_iterator I1 = CompStmt1->body_begin();
+ CompoundStmt::const_body_iterator I2 = CompStmt2->body_begin();
+ while (I1 != CompStmt1->body_end() && I2 != CompStmt2->body_end()) {
+ if (!isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+ return false;
+ ++I1;
+ ++I2;
+ }
+
+ return true;
+ }
+ case Stmt::CompoundAssignOperatorClass:
case Stmt::BinaryOperatorClass: {
- const BinaryOperator *BinOp1 = cast<BinaryOperator>(Expr1);
- const BinaryOperator *BinOp2 = cast<BinaryOperator>(Expr2);
+ const BinaryOperator *BinOp1 = cast<BinaryOperator>(Stmt1);
+ const BinaryOperator *BinOp2 = cast<BinaryOperator>(Stmt2);
return BinOp1->getOpcode() == BinOp2->getOpcode();
}
case Stmt::CharacterLiteralClass: {
- const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Expr1);
- const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Expr2);
+ const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Stmt1);
+ const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Stmt2);
return CharLit1->getValue() == CharLit2->getValue();
}
case Stmt::DeclRefExprClass: {
- const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Expr1);
- const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Expr2);
+ const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Stmt1);
+ const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Stmt2);
return DeclRef1->getDecl() == DeclRef2->getDecl();
}
case Stmt::IntegerLiteralClass: {
- const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Expr1);
- const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Expr2);
+ const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Stmt1);
+ const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Stmt2);
return IntLit1->getValue() == IntLit2->getValue();
}
case Stmt::FloatingLiteralClass: {
- const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Expr1);
- const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Expr2);
+ const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Stmt1);
+ const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Stmt2);
return FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue());
}
+ case Stmt::StringLiteralClass: {
+ const StringLiteral *StringLit1 = cast<StringLiteral>(Stmt1);
+ const StringLiteral *StringLit2 = cast<StringLiteral>(Stmt2);
+ return StringLit1->getString() == StringLit2->getString();
+ }
case Stmt::MemberExprClass: {
- const MemberExpr *MemberExpr1 = cast<MemberExpr>(Expr1);
- const MemberExpr *MemberExpr2 = cast<MemberExpr>(Expr2);
- return MemberExpr1->getMemberDecl() == MemberExpr2->getMemberDecl();
+ const MemberExpr *MemberStmt1 = cast<MemberExpr>(Stmt1);
+ const MemberExpr *MemberStmt2 = cast<MemberExpr>(Stmt2);
+ return MemberStmt1->getMemberDecl() == MemberStmt2->getMemberDecl();
}
case Stmt::UnaryOperatorClass: {
- const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Expr1);
- const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Expr2);
+ const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Stmt1);
+ const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Stmt2);
return UnaryOp1->getOpcode() == UnaryOp2->getOpcode();
}
}
Modified: cfe/trunk/test/Analysis/identical-expressions.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/identical-expressions.cpp?rev=201701&r1=201700&r2=201701&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/identical-expressions.cpp (original)
+++ cfe/trunk/test/Analysis/identical-expressions.cpp Wed Feb 19 11:44:11 2014
@@ -1043,6 +1043,11 @@ void test_float() {
a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
}
+const char *test_string() {
+ float a = 0;
+ return a > 5 ? "abc" : "abc"; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
void test_unsigned_expr() {
unsigned a = 0;
unsigned b = 0;
@@ -1158,3 +1163,149 @@ void test_signed_nested_cond_expr() {
int c = 3;
a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 4 : 4); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
}
+
+void test_identical_branches1(bool b) {
+ int i = 0;
+ if (b) { // expected-warning {{true and false branches are identical}}
+ ++i;
+ } else {
+ ++i;
+ }
+}
+
+void test_identical_branches2(bool b) {
+ int i = 0;
+ if (b) { // expected-warning {{true and false branches are identical}}
+ ++i;
+ } else
+ ++i;
+}
+
+void test_identical_branches3(bool b) {
+ int i = 0;
+ if (b) { // no warning
+ ++i;
+ } else {
+ i++;
+ }
+}
+
+void test_identical_branches4(bool b) {
+ int i = 0;
+ if (b) { // expected-warning {{true and false branches are identical}}
+ } else {
+ }
+}
+
+void test_identical_branches_break(bool b) {
+ while (true) {
+ if (b) // expected-warning {{true and false branches are identical}}
+ break;
+ else
+ break;
+ }
+}
+
+void test_identical_branches_continue(bool b) {
+ while (true) {
+ if (b) // expected-warning {{true and false branches are identical}}
+ continue;
+ else
+ continue;
+ }
+}
+
+void test_identical_branches_func(bool b) {
+ if (b) // expected-warning {{true and false branches are identical}}
+ func();
+ else
+ func();
+}
+
+void test_identical_branches_func_arguments(bool b) {
+ if (b) // no-warning
+ funcParam(1);
+ else
+ funcParam(2);
+}
+
+void test_identical_branches_cast1(bool b) {
+ long v = -7;
+ if (b) // no-warning
+ v = (signed int) v;
+ else
+ v = (unsigned int) v;
+}
+
+void test_identical_branches_cast2(bool b) {
+ long v = -7;
+ if (b) // expected-warning {{true and false branches are identical}}
+ v = (signed int) v;
+ else
+ v = (signed int) v;
+}
+
+int test_identical_branches_return_int(bool b) {
+ int i = 0;
+ if (b) { // expected-warning {{true and false branches are identical}}
+ i++;
+ return i;
+ } else {
+ i++;
+ return i;
+ }
+}
+
+int test_identical_branches_return_func(bool b) {
+ if (b) { // expected-warning {{true and false branches are identical}}
+ return func();
+ } else {
+ return func();
+ }
+}
+
+void test_identical_branches_for(bool b) {
+ int i;
+ int j;
+ if (b) { // expected-warning {{true and false branches are identical}}
+ for (i = 0, j = 0; i < 10; i++)
+ j += 4;
+ } else {
+ for (i = 0, j = 0; i < 10; i++)
+ j += 4;
+ }
+}
+
+void test_identical_branches_while(bool b) {
+ int i = 10;
+ if (b) { // expected-warning {{true and false branches are identical}}
+ while (func())
+ i--;
+ } else {
+ while (func())
+ i--;
+ }
+}
+
+void test_identical_branches_do_while(bool b) {
+ int i = 10;
+ if (b) { // expected-warning {{true and false branches are identical}}
+ do {
+ i--;
+ } while (func());
+ } else {
+ do {
+ i--;
+ } while (func());
+ }
+}
+
+void test_identical_branches_if(bool b, int i) {
+ if (b) { // expected-warning {{true and false branches are identical}}
+ if (i < 5)
+ i += 10;
+ } else {
+ if (i < 5)
+ i += 10;
+ }
+}
Modified: cfe/trunk/test/Analysis/misc-ps-region-store.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/misc-ps-region-store.cpp?rev=201701&r1=201700&r2=201701&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/misc-ps-region-store.cpp (original)
+++ cfe/trunk/test/Analysis/misc-ps-region-store.cpp Wed Feb 19 11:44:11 2014
@@ -588,6 +588,7 @@ void rdar11401827() {
int x = int();
if (!x) {
clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+ ; // Suppress warning that both branches are identical
}
else {
clang_analyzer_warnIfReached(); // no-warning
More information about the cfe-commits
mailing list