Hi, Jordan.
On Thu, 2014-01-30 at 09:47 -0800, Jordan Rose wrote:
> Hi, Daniel. Can you split these two checks up into separate patches? I
> know they're pretty similar and have the same basic idea, but we like
> to have commits be limited to one new feature at a time. (Combining
> bitwise and logical operators into one is fine, though.) Sorry it's a
> bit more work for you to tease them apart.
No problem. I have fixed your comments with the two attached patches.
if_stmt adds support to test if both branches are identical and should
be applied first. bin_op adds support to find identical expressions when
using binary operators.
> + // Split operators as long as we still have operators to split on. We will
> + // get called for every binary operator in an expression so there is no
> need
> + // to check every one against each other here, just the right most one with
> + // the others.
>
> Clever! Too bad it comes out to an N^2 algorithm, but N is likely
> pretty small most of the time, and the nature of isIdenticalStmt
Thanks. I thought about a better way to fix this when I implemented it,
but couldn't find a good one. First, somehow, sorting the expressions
and check only adjacent ones will probably be slower in practice due to
the overhead of sorting.
> Some missing test cases:
>
> Should not warn: x == 4 && y == 5 || x == 4 && y == 6
>
> Should warn: x ? "abc" : "abc"
>
> Test cases with while, do, for, if, and return within the branches of
> an if. If you add the code, we need to test them! We could also not
> worry about this for now and just say the checker isn't powerful enough
> to handle them.
Test cases added.
> Why would that be useful? Because I'm a bit concerned about performance
> for the if-branch-check. Can you try running this over a large-ish
> project and let me know the before and after times of analyzing with
> this checker on? If it's a significant difference we should put the
> if-check under a separate checker name.
I ran some tests with php 5.5.8 and openssl 1.0.1f. The tests were done
on my laptop with a ssd and 8GB of ram, so the numbers should be cpu
bound. Between each run I completely removed the directory and
reconfigured to make sure conditions were as similar as possible. The
checker was modified t only check the branches of an if statement, not
the binary operators.
Adding the check when running scan-build on php increased the time by
0.2%. On openssl it actually decreased by 0.15% instead. I think the
background tasks of my otherwise idle desktop has more influence on the
numbers than the test.
php 5.5.8 base run:
real 53m44.366s
user 51m31.245s
sys 1m11.288s
php 5.5.8 with checking if stmts:
real 53m51.311s
user 51m35.985s
sys 1m12.949s
openssl 1.0.1f base run:
real 11m40.819s
user 10m39.124s
sys 0m49.091s
openssl 1.0.1f with checking if stmts:
real 11m39.783s
user 10m38.536s
sys 0m48.879s
Best regards,
Daniel Fahlgren
diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
index faa8cd5..ad9b6cc 100644
--- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -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.
//===----------------------------------------------------------------------===//
@@ -39,8 +39,10 @@ public:
explicit FindIdenticalExprVisitor(BugReporter &B, AnalysisDeclContext *A)
: BR(B), 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:
@@ -49,6 +51,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(),
+ "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))
@@ -104,7 +139,7 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
// 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;
@@ -127,7 +162,7 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
// 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(
@@ -148,7 +183,7 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
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
@@ -156,81 +191,183 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
/// 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))
+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 either expression comes from a macro then don't warn even if
- // the expressions are identical.
- if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
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))
+
+ 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;
- ++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())
- return false;
- if (I2 != Expr2->child_end())
- return false;
- switch (Expr1->getStmtClass()) {
+ 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();
}
}
diff --git a/test/Analysis/identical-expressions.cpp b/test/Analysis/identical-expressions.cpp
index 3a331fb..70bd12c 100644
--- a/test/Analysis/identical-expressions.cpp
+++ b/test/Analysis/identical-expressions.cpp
@@ -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,142 @@ 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_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;
+ }
+}
diff --git a/test/Analysis/misc-ps-region-store.cpp b/test/Analysis/misc-ps-region-store.cpp
index 6a873f0..388a769 100644
--- a/test/Analysis/misc-ps-region-store.cpp
+++ b/test/Analysis/misc-ps-region-store.cpp
@@ -588,6 +588,7 @@ void rdar11401827() {
int x = int();
if (!x) {
clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+ ++x; // Suppress warning that both branches are identical
}
else {
clang_analyzer_warnIfReached(); // no-warning
diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
index ad9b6cc..22d3d37 100644
--- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -48,9 +48,59 @@ public:
private:
BugReporter &BR;
AnalysisDeclContext *AC;
+
+ void reportIdenticalExpr(const BinaryOperator *B, bool CheckBitwise,
+ ArrayRef<SourceRange> Sr);
+ void checkBitwiseOrLogicalOp(const BinaryOperator *B, bool CheckBitwise);
+ void checkComparisionOp(const BinaryOperator *B);
};
} // end anonymous namespace
+void FindIdenticalExprVisitor::reportIdenticalExpr(const BinaryOperator *B,
+ bool CheckBitwise,
+ ArrayRef<SourceRange> Sr) {
+ StringRef Message;
+ if (CheckBitwise)
+ Message = "identical expressions on both sides of bitwise operator";
+ else
+ Message = "identical expressions on both sides of logical operator";
+
+ PathDiagnosticLocation ELoc =
+ PathDiagnosticLocation::createOperatorLoc(B, BR.getSourceManager());
+ BR.EmitBasicReport(AC->getDecl(),
+ "Use of identical expressions",
+ categories::LogicError,
+ Message, ELoc, Sr);
+}
+
+void FindIdenticalExprVisitor::checkBitwiseOrLogicalOp(const BinaryOperator *B,
+ bool CheckBitwise) {
+ SourceRange Sr[2];
+
+ const Expr *LHS = B->getLHS();
+ const Expr *RHS = B->getRHS();
+
+ // Split operators as long as we still have operators to split on. We will
+ // get called for every binary operator in an expression so there is no need
+ // to check every one against each other here, just the right most one with
+ // the others.
+ while (const BinaryOperator *B2 = dyn_cast<BinaryOperator>(LHS)) {
+ if (B->getOpcode() != B2->getOpcode())
+ break;
+ if (isIdenticalStmt(AC->getASTContext(), RHS, B2->getRHS())) {
+ Sr[0] = RHS->getSourceRange();
+ Sr[1] = B2->getRHS()->getSourceRange();
+ reportIdenticalExpr(B, CheckBitwise, Sr);
+ }
+ LHS = B2->getLHS();
+ }
+ if (isIdenticalStmt(AC->getASTContext(), RHS, LHS)) {
+ Sr[0] = RHS->getSourceRange();
+ Sr[1] = LHS->getSourceRange();
+ reportIdenticalExpr(B, CheckBitwise, Sr);
+ }
+}
+
bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) {
const Stmt *Stmt1 = I->getThen();
const Stmt *Stmt2 = I->getElse();
@@ -86,8 +136,25 @@ bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) {
bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
BinaryOperator::Opcode Op = B->getOpcode();
- if (!BinaryOperator::isComparisonOp(Op))
- return true;
+
+ if (BinaryOperator::isBitwiseOp(Op))
+ checkBitwiseOrLogicalOp(B, true);
+
+ if (BinaryOperator::isLogicalOp(Op))
+ checkBitwiseOrLogicalOp(B, false);
+
+ if (BinaryOperator::isComparisonOp(Op))
+ checkComparisionOp(B);
+
+ // We want to visit ALL nodes (subexpressions of binary comparison
+ // expressions too) that contains comparison operators.
+ // True is always returned to traverse ALL nodes.
+ return true;
+}
+
+void FindIdenticalExprVisitor::checkComparisionOp(const BinaryOperator *B) {
+ BinaryOperator::Opcode Op = B->getOpcode();
+
//
// Special case for floating-point representation.
//
@@ -120,21 +187,21 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
(DeclRef2->getType()->hasFloatingRepresentation())) {
if (DeclRef1->getDecl() == DeclRef2->getDecl()) {
if ((Op == BO_EQ) || (Op == BO_NE)) {
- return true;
+ return;
}
}
}
} else if ((FloatLit1) && (FloatLit2)) {
if (FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue())) {
if ((Op == BO_EQ) || (Op == BO_NE)) {
- return true;
+ return;
}
}
} else if (LHS->getType()->hasFloatingRepresentation()) {
// If any side of comparison operator still has floating-point
// representation, then it's an expression. Don't warn.
// Here only LHS is checked since RHS will be implicit casted to float.
- return true;
+ return;
} else {
// No special case with floating-point representation, report as usual.
}
@@ -150,10 +217,6 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
BR.EmitBasicReport(AC->getDecl(), "Compare of identical expressions",
categories::LogicError, Message, ELoc);
}
- // We want to visit ALL nodes (subexpressions of binary comparison
- // expressions too) that contains comparison operators.
- // True is always returned to traverse ALL nodes.
- return true;
}
bool FindIdenticalExprVisitor::VisitConditionalOperator(
diff --git a/test/Analysis/identical-expressions.cpp b/test/Analysis/identical-expressions.cpp
index 70bd12c..ad16bac 100644
--- a/test/Analysis/identical-expressions.cpp
+++ b/test/Analysis/identical-expressions.cpp
@@ -1302,3 +1302,74 @@ void test_identical_branches_if(bool b, int i) {
i += 10;
}
}
+
+void test_identical_bitwise1() {
+ int a = 5 | 5; // expected-warning {{identical expressions on both sides of bitwise operator}}
+}
+
+void test_identical_bitwise2() {
+ int a = 5;
+ int b = a | a; // expected-warning {{identical expressions on both sides of bitwise operator}}
+}
+
+void test_identical_bitwise3() {
+ int a = 5;
+ int b = (a | a); // expected-warning {{identical expressions on both sides of bitwise operator}}
+}
+
+void test_identical_bitwise4() {
+ int a = 4;
+ int b = a | 4; // no-warning
+}
+
+void test_identical_bitwise5() {
+ int a = 4;
+ int b = 4;
+ int c = a | b; // no-warning
+}
+
+void test_identical_bitwise6() {
+ int a = 5;
+ int b = a | 4 | a; // expected-warning {{identical expressions on both sides of bitwise operator}}
+}
+
+void test_identical_bitwise7() {
+ int a = 5;
+ int b = func() | func(); // no-warning
+}
+
+void test_identical_logical1(int a) {
+ if (a == 4 && a == 4) // expected-warning {{identical expressions on both sides of logical operator}}
+ ;
+}
+
+void test_identical_logical2(int a) {
+ if (a == 4 || a == 5 || a == 4) // expected-warning {{identical expressions on both sides of logical operator}}
+ ;
+}
+
+void test_identical_logical3(int a) {
+ if (a == 4 || a == 5 || a == 6) // no-warning
+ ;
+}
+
+void test_identical_logical4(int a) {
+ if (a == func() || a == func()) // no-warning
+ ;
+}
+
+void test_identical_logical5(int x, int y) {
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wlogical-op-parentheses"
+ if (x == 4 && y == 5 || x == 4 && y == 6) // no-warning
+ ;
+#pragma clang diagnostic pop
+}
+
+void test_identical_logical6(int x, int y) {
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wlogical-op-parentheses"
+ if (x == 4 && y == 5 || x == 4 && y == 5) // expected-warning {{identical expressions on both sides of logical operator}}
+ ;
+#pragma clang diagnostic pop
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
