Hi Jordan,
On Mon, 2014-02-03 at 18:37 -0800, Jordan Rose wrote:
> Thanks for running the timing checks. I agree that this seems pretty
> benign.
>
> I thought of another controversial test case:
> if (foo()) {
> doSomething();
> } else if (bar()) {
> /* do nothing */
> } else if (baz()) {
> /* do nothing */
> } else {
> error();
> }
> It'd be pretty easy to just not check empty compound-statements. What
> do you think?
I fail to understand the problem. Can you elaborate, please? If we
rearrange the code a bit we get
if (foo()) {
doSomething();
} else
if (bar()) {
/* do nothing */
} else
if (baz()) {
/* do nothing */
} else {
error();
}
with no two if-then-else-branches identical. The current code doesn't
(correctly?) warn about this.
A future addition might be to detect the same logical expressions in
multiple nested if statements, like:
if (i == 1) {
doSomething1();
} else if (i == 2) {
doSomething2();
} else if (i == 2) {
doSomething3();
} else {
error();
}
>
> More test cases for the logical op checker:
> x == 4 && y == 5 || x == 4
> x == 4 || y == 5 && x == 4
> x == 4 || x == 4 && y == 5
>
> All of these would be fair to warn on, but we can probably get away
> with not for the first pass. If we already support one, though, should
> we add it as a regression test?
Unfortunately we don't support that right now, but I agree it would be a
good addition.
> Finally, the typo "comparision" is still in the patch. :-)
I should have paid more attention. Sorry :-) Attached is a new version
of the second patch.
>
Cheers,
Daniel Fahlgren
diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
index ad9b6cc..0ae7a1e 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 checkComparisonOp(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))
+ checkComparisonOp(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::checkComparisonOp(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
