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

[Jordan Rose]

On Mar 6, 2014, at 13:16 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

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

Ah, that's also me, probably. The original list was set up by Anna and Anton Yartsev, for the most part, but Anna is still out and I don't think Anton wants to be the general maintainer for it. The fastest way to get it updated is to send me a diff; the entire analyzer site is in www/analyzer/.

Patch committed as r203585!

Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140311/6d857c13/attachment.html>