[PATCH][StaticAnalyzer] new check comparing equal expression

[Per Viberg]

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<mailto:Per.Viberg at evidente.se>

www.evidente.se<http://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<http://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

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