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

[Daniel Fahlgren]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: chained_if_stmts_2.patch
Type: text/x-patch
Size: 3727 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140306/bc1a7459/attachment.bin>