[PATCH][StaticAnalyzer] new check comparing equal expression

Tue Nov 5 09:26:22 PST 2013

I got a chance to look at the results of analyzing LLVM/Clang with this checker, and also found zero reports—true or false positives. On the one hand, these mistakes probably don't last long in an open-source project. On the other, is it worth having a checker that catches problems that don't show up in the real world? Anna?

(The answer is probably "yes", but I thought I might as well bring it up. Perhaps it's disabled in our "shallow" analysis mode.)
Jordan

On Oct 31, 2013, at 3:20 , Per Viberg <Per.Viberg at evidente.se> wrote:

> 
> Hi Jordan, 
> 
> here is the new patch changed according to your comments below. I have renamed the check from EqualCompareChecker to IdenticalExpChecker to accommodate for upcoming tests in this category. I also ran the analysis on an open source project called cppchecker to check performance.
> 
> Without IdenticalExpChecker: 5197  seconds
> With IdenticalExpChecker: 5239 seconds.
> Result: approximately 40 seconds longer, which amounts to < 1% increase in analyse time.
> 
> Depending on the coding style of the source code analysed, this could differ dramatically. If someone uses a lot of sub-expressions within sub-expression the analysis time increases, but then this check becomes more important to do since the error will be more difficult to detect by manual review.
> 
> The checker didn't find any "real" errors in the cppChecker project, and not any false positives.
> 
> best regards,
> Per
> .......................................................................................................................
> Per Viberg Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> Phone:    +46 (0)8 402 79 00
> Mobile:    +46 (0)70 912 42 52
> E-mail:     Per.Viberg at evidente.se 
> 
> www.evidente.se
> This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.
> Från: Jordan Rose [jordan_rose at apple.com]
> Skickat: den 11 oktober 2013 02:43
> Till: Per Viberg
> Cc: cfe-commits at cs.uiuc.edu
> Ämne: Re: [PATCH][StaticAnalyzer] new check comparing equal expression
> 
> Hi, Per. Looking good; more comments for you. The performance thing is also something I'd like to get resolved soon: how long does this checker alone take over a large body of code, compared to the existing path-sensitive analysis? (It looks like it's linear on average, quadratic at worst, so I'd expect it to be much cheaper, but still.)
> 
> 
> +static bool isSameExpr(const ASTContext &Ctx, Expr *Expr1, Expr *Expr2);
> 
> const Expr *, please?
> 
> 
> +  // only visit nodes that are binary expressions
> 
> LLVM convention says that all comments should be complete sentences, including capitalization and a final period.
> 
> 
> +  bool isSameExpression;
> 
> No reason to declare this way ahead of time. Please move this down to where it's actually used—or eliminate it altogether, and just put the call in the if-condition.
> 
> 
> +  DeclRefExpr *declref1 = dyn_cast<DeclRefExpr>(LHS);
> +  DeclRefExpr *declref2 = dyn_cast<DeclRefExpr>(RHS);
> 
> LLVM convention says that all variable names are UpperCamelCased. There are some of these in isSameExpr as well.
> 
> 
> +    // If any side of comparison still has floatingpointrepresentation,
> +    // then it's an expression-> don't warn.
> +    // (here only LHS is checked since RHS will be implicit casted to float)
> 
> Please turn these into complete sentences (and split up "floating-point representation").
> 
> 
> +  } else { // No special case with floating point representation, report as
> +           // usual
> +  }
> 
> I get that it's important to call out this case, but please put the comment on its own line within the body of the if.
> 
> 
> +// t*(u + t) and t*u + t*t are not considered equal.
> 
> I wouldn't even worry about this. This is a pretty unlikely typo or copy-paste error. Also, no reason not to make this whole block a proper Doxygen comment. Even if it's not a public function, IDEs can still make use of the comment if it's marked up as Doxygen (and with ///).
> 
> 
> Has this checker found any real errors in code you have access to? Has it found any false positives?
> 
> Thanks for working on this!
> Jordan
> 
> 
> On Oct 10, 2013, at 8:42 , Per Viberg <Per.Viberg at evidente.se> wrote:
> 
> > 
> > Hello,
> > 
> > changed patch according to review comments.
> > 
> > Please review revised patch. 
> > 
> > The patch adds a check that warns for comparison of equal expressions.
> > example:
> > x + 1 == x +1;
> > 
> > best regards,
> > Per
> > 
> > .......................................................................................................................
> > Per Viberg Senior Engineer
> > Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> > Phone:    +46 (0)8 402 79 00
> > Mobile:    +46 (0)70 912 42 52
> > E-mail:     Per.Viberg at evidente.se
> > 
> > www.evidente.se
> > This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.
> > _______________________________________________
> > 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131105/447c2218/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IdenticalExpr_rev3.diff
Type: text/x-patch
Size: 34391 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131105/447c2218/attachment.bin>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131105/447c2218/attachment-0001.html>