[analyzer] Extend IdenticalExprChecker to check logical and bitwise expressions.

IdenticalExprChecker now warns if any expressions in a logical or bitwise
chain (&&, ||, &, |, or ^) are the same. Unlike the previous patch, this
actually checks all subexpressions against each other (an O(N^2) operation,
but N is likely to be small).

Patch by Daniel Fahlgren!

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@201702 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
index 6c6f472f3f70ae4d92943486c4c9c20156af18aa..a76dff120bebc7697ef928e293f4ffdd2f228e39 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -35,6 +35,9 @@ static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
 namespace {
 class FindIdenticalExprVisitor
     : public RecursiveASTVisitor<FindIdenticalExprVisitor> {
+  BugReporter &BR;
+  const CheckerBase *Checker;
+  AnalysisDeclContext *AC;
 public:
   explicit FindIdenticalExprVisitor(BugReporter &B,
                                     const CheckerBase *Checker,
@@ -48,12 +51,59 @@ public:
   bool VisitConditionalOperator(const ConditionalOperator *C);
 
 private:
-  BugReporter &BR;
-  const CheckerBase *Checker;
-  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(), Checker,
+                     "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();
@@ -89,8 +139,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.
   //
@@ -123,21 +190,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.
   }
@@ -154,10 +221,6 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
                        "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 acb7c8c57b0648a36d53bee39b69536be424ea8f..950cdd93ff4d99f6b7c7e30d62cfc1d4cd071f47 100644 (file)
--- a/test/Analysis/identical-expressions.cpp
+++ b/test/Analysis/identical-expressions.cpp
@@ -1309,3 +1309,89 @@ 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
+    ;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wlogical-op-parentheses"
+void test_identical_logical5(int x, int y) {
+  if (x == 4 && y == 5 || x == 4 && y == 6) // no-warning
+    ;
+}
+
+void test_identical_logical6(int x, int y) {
+  if (x == 4 && y == 5 || x == 4 && y == 5) // expected-warning {{identical expressions on both sides of logical operator}}
+    ;
+}
+
+void test_identical_logical7(int x, int y) {
+  // FIXME: We should warn here
+  if (x == 4 && y == 5 || x == 4)
+    ;
+}
+
+void test_identical_logical8(int x, int y) {
+  // FIXME: We should warn here
+  if (x == 4 || y == 5 && x == 4)
+    ;
+}
+
+void test_identical_logical9(int x, int y) {
+  // FIXME: We should warn here
+  if (x == 4 || x == 4 && y == 5)
+    ;
+}
+#pragma clang diagnostic pop