[PATCH][StaticAnalyzer] new check comparing equal expression

[Jordan Rose]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: equalcompare_rev5.diff
Type: text/x-patch
Size: 33773 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131010/f9669c25/attachment.bin>
-------------- next part --------------
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits