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

[Jordan Rose]

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).

+        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

Thanks for working on this!
Jordan


On Mar 5, 2014, at 9:36 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi,
> 
> ping, any comments?
> 
> Best regards,
> Daniel Fahlgren
> 
> On Fri, 2014-02-21 at 08:16 +0100, Daniel Fahlgren wrote:
>> 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
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits