Hi Jordan,
Thanks for reviewing this.
On Wed, 2014-03-05 at 14:38 -0800, Jordan Rose wrote:
> I'm sorry, I managed to miss this one. Please feel free to include me
> in the To or CC fields when you have analyzer patches.
> I'm glad to get this check! It definitely seems useful for copy-paste
> checking. Thanks for doing the timing checks in advance.
>
> Comments:
>
> + if (Stmt1 && Stmt2) {
> + const Expr *Cond1 = I->getCond();
> + const Stmt *Else = Stmt2;
> + while (const IfStmt *I2 = dyn_cast<IfStmt>(Else)) {
>
> If you use dyn_cast_or_null here, then you can drop the check for a
> missing 'else' at the end of the loop (and at the beginning of the loop
> too, though that's less interesting).
I've kept the check at the beginning, mostly because I think it makes
the code more readable.
> + PathDiagnosticLocation ELoc =
> PathDiagnosticLocation::createBegin(Cond2,
> + BR.getSourceManager(), AC);
>
> Please use the regular Stmt-based constructor for
> PathDiagnosticLocation. We want to consider the whole condition to be
> the location, not just the beginning.
>
> + BR.EmitBasicReport(AC->getDecl(), Checker,
> + "Identical conditions",
> + categories::LogicError,
> + "identical condition as a previous one", ELoc,
> Sr);
>
> This isn't quite valid English: two things can "be identical", and one
> thing can "be identical to" another thing, but one thing can't be
> "identical as" another thing. The "one" also makes me
> uncomfortable...it feels weird in a compiler tool. How about
> "expression is identical to previous condition"? (I'm cheating by using
> "expression" to mean "condition" in this case, but it sounds better to
> not repeat "condition".)
>
> As far as test cases go, please include some cases where
> - the duplicated condition is not last
> - the duplicated condition is not first
> - there are multiple pairs of duplicated conditions in the chain
Valid points as usual. Attached is an updated version of the patch.
Btw, who should I poke to get the list of potential checkers updated?
The page still lists different.IdenticalExprBinOp,
different.IdenticalStmtThenElse and different.CondOpIdenticalReturn as
without progress.
Cheers,
Daniel Fahlgren
Index: lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp (revision 203158)
+++ lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp (working copy)
@@ -108,6 +108,30 @@
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_or_null<IfStmt>(Else)) {
+ const Expr *Cond2 = I2->getCond();
+ if (isIdenticalStmt(AC->getASTContext(), Cond1, Cond2, false)) {
+ SourceRange Sr = Cond1->getSourceRange();
+ PathDiagnosticLocation ELoc(Cond2, BR.getSourceManager(), AC);
+ BR.EmitBasicReport(AC->getDecl(), Checker, "Identical conditions",
+ categories::LogicError,
+ "expression is identical to previous condition",
+ ELoc, Sr);
+ }
+ Else = I2->getElse();
+ }
+ }
+
if (!Stmt1 || !Stmt2)
return true;
Index: test/Analysis/identical-expressions.cpp
===================================================================
--- test/Analysis/identical-expressions.cpp (revision 203158)
+++ test/Analysis/identical-expressions.cpp (working copy)
@@ -1406,3 +1406,108 @@
;
}
#pragma clang diagnostic pop
+
+void test_warn_chained_if_stmts_1(int x) {
+ if (x == 1)
+ ;
+ else if (x == 1) // expected-warning {{expression is identical to previous condition}}
+ ;
+}
+
+void test_warn_chained_if_stmts_2(int x) {
+ if (x == 1)
+ ;
+ else if (x == 1) // expected-warning {{expression is identical to previous condition}}
+ ;
+ else if (x == 1) // expected-warning {{expression is identical to previous condition}}
+ ;
+}
+
+void test_warn_chained_if_stmts_3(int x) {
+ if (x == 1)
+ ;
+ else if (x == 2)
+ ;
+ else if (x == 1) // expected-warning {{expression is identical to previous condition}}
+ ;
+}
+
+void test_warn_chained_if_stmts_4(int x) {
+ if (x == 1)
+ ;
+ else if (func())
+ ;
+ else if (x == 1) // expected-warning {{expression is identical to previous condition}}
+ ;
+}
+
+void test_warn_chained_if_stmts_5(int x) {
+ if (x & 1)
+ ;
+ else if (x & 1) // expected-warning {{expression is identical to previous condition}}
+ ;
+}
+
+void test_warn_chained_if_stmts_6(int x) {
+ if (x == 1)
+ ;
+ else if (x == 2)
+ ;
+ else if (x == 2) // expected-warning {{expression is identical to previous condition}}
+ ;
+ else if (x == 3)
+ ;
+}
+
+void test_warn_chained_if_stmts_7(int x) {
+ if (x == 1)
+ ;
+ else if (x == 2)
+ ;
+ else if (x == 3)
+ ;
+ else if (x == 2) // expected-warning {{expression is identical to previous condition}}
+ ;
+ else if (x == 5)
+ ;
+}
+
+void test_warn_chained_if_stmts_8(int x) {
+ if (x == 1)
+ ;
+ else if (x == 2)
+ ;
+ else if (x == 3)
+ ;
+ else if (x == 2) // expected-warning {{expression is identical to previous condition}}
+ ;
+ else if (x == 5)
+ ;
+ else if (x == 3) // expected-warning {{expression is identical to previous condition}}
+ ;
+ else if (x == 7)
+ ;
+}
+
+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
