[PATCH] Detect identical conditions in if-else-if statements

[Daniel Fahlgren]

Hi,

This patch makes the IdenticalExprChecker warn about code like:

if (i == 1) {
  foo1();
} else if (i == 1) {
  foo2();
}

The logic is the same as for binary operators. We only need to check the
current if statement and the following ones. Just as with the binary
operator check that will be O(n^2), but if you have a very large number
of chained if-else-if statements that might be a reason for a warning of
its own ;)

I ran scan-build on PHP and openssl and didn't notice any major increase
in build time. For PHP it took 1% longer, and openssl was 0.3% faster
with this checker enabled. That includes all other checks done by
alpha.core.IdenticalExpr.

Cheers,
Daniel Fahlgren

Index: lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp	(revision 201839)
+++ lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp	(working copy)
@@ -108,6 +108,32 @@
   const Stmt *Stmt1 = I->getThen();
   const Stmt *Stmt2 = I->getElse();
 
+  // Check for identical conditions:
+  //
+  // if (b) {
+  //   foo1();
+  // } else if (b) {
+  //   foo2();
+  // }
+  if (Stmt1 && Stmt2) {
+    const Expr *Cond1 = I->getCond();
+    const Stmt *Else = Stmt2;
+    while (const IfStmt *I2 = dyn_cast<IfStmt>(Else)) {
+      const Expr *Cond2 = I2->getCond();
+      if (isIdenticalStmt(AC->getASTContext(), Cond1, Cond2, false)) {
+        SourceRange Sr = Cond1->getSourceRange();
+        PathDiagnosticLocation ELoc = PathDiagnosticLocation::createBegin(Cond2,
+            BR.getSourceManager(), AC);
+        BR.EmitBasicReport(AC->getDecl(), Checker,
+                           "Identical conditions",
+                           categories::LogicError,
+                           "identical condition as a previous one", ELoc, Sr);
+      }
+      if (!(Else = I2->getElse()))
+        break;
+    }
+  }
+
   if (!Stmt1 || !Stmt2)
     return true;
 
Index: test/Analysis/identical-expressions.cpp
===================================================================
--- test/Analysis/identical-expressions.cpp	(revision 201839)
+++ test/Analysis/identical-expressions.cpp	(working copy)
@@ -1406,3 +1406,67 @@
     ;
 }
 #pragma clang diagnostic pop
+
+void test_warn_chained_if_stmts_1(int x) {
+  if (x == 1)
+    ;
+  else if (x == 1) // expected-warning {{identical condition as a previous one}}
+    ;
+}
+
+void test_warn_chained_if_stmts_2(int x) {
+  if (x == 1)
+    ;
+  else if (x == 1) // expected-warning {{identical condition as a previous one}}
+    ;
+  else if (x == 1) // expected-warning {{identical condition as a previous one}}
+    ;
+}
+
+void test_warn_chained_if_stmts_3(int x) {
+  if (x == 1)
+    ;
+  else if (x == 2)
+    ;
+  else if (x == 1) // expected-warning {{identical condition as a previous one}}
+    ;
+}
+
+void test_warn_chained_if_stmts_4(int x) {
+  if (x == 1)
+    ;
+  else if (func())
+    ;
+  else if (x == 1) // expected-warning {{identical condition as a previous one}}
+    ;
+}
+
+void test_warn_chained_if_stmts_5(int x) {
+  if (x & 1)
+    ;
+  else if (x & 1) // expected-warning {{identical condition as a previous one}}
+    ;
+}
+
+void test_nowarn_chained_if_stmts_1(int x) {
+  if (func())
+    ;
+  else if (func()) // no-warning
+    ;
+}
+
+void test_nowarn_chained_if_stmts_2(int x) {
+  if (func())
+    ;
+  else if (x == 1)
+    ;
+  else if (func()) // no-warning
+    ;
+}
+
+void test_nowarn_chained_if_stmts_3(int x) {
+  if (x++)
+    ;
+  else if (x++) // no-warning
+    ;
+}

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits