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

[Anton Yartsev]

On 11.03.2014 20:58, Jordan Rose wrote:
>
> On Mar 6, 2014, at 13:16 , Daniel Fahlgren <daniel at fahlgren.se 
> <mailto: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

Hm, the list is really long outdated. I'll update it one of these days.

-- 
Anton

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140312/ca733cf7/attachment.html>