[PATCH][StaticAnalyzer] new check comparing equal expression

[Jordan Rose]

Hi, Per. This does look very good, though like Hans said you should probably run it through clang-format to make sure it matches up with the LLVM style. Additionally, I think this is better as a syntactic checker, using RecursiveASTVisitor: the same advice I gave Anders for the StringPlusChar checker, and the approach he took for the SizeofOnExpression checker.

Beyond that, some local comments:

+//== EqualCompareChecker.cpp - Compare equal expression checker--*- C++ -*--==//

Nitpick: put a space after the summary. Actually, since the file's extension is .cpp, you don't need the -*- C++ -*- part at all...you can just fill the rest of the line with dashes.


+// This defines EqualCompareChecker, a check that
+// warns if equal expression on both side of compare in statement.

Please make this a Doxygen file comment, as described here: http://llvm.org/docs/CodingStandards.html#file-headers


+      return !unaryop1->isIncrementDecrementOp();

Even though this actually does something ("++x == ++x"), it's not well-defined anyway (specifically, either one could go first), so I'd still like to catch this as a copy-paste error. What do you think?


+  if (Op != BO_LT &&
+      Op != BO_GT &&
+      Op != BO_LE &&
+      Op != BO_GE &&
+      Op != BO_EQ &&
+      Op != BO_NE)
+    return;

There's shorthand for this: BinaryOperator::isComparisonOp.


+  /*
+   * Special case for floating point representation.
+   *

We prefer single comments in source, even for long code blocks. http://llvm.org/docs/CodingStandards.html#comment-formatting


+  Expr* LHS = B->getLHS()->IgnoreParenImpCasts();

In general in the analyzer we prefer to make as many AST pointers const as we can (i.e. "const Expr *LHS").


+  else if ((floatlit1)&&(floatlit2)) {

I can't see anyone ever writing "if (0.9 == 1.0)". Let's not worry about the two-floating-point-literals case.


+    if (((Op == BO_EQ)||(Op == BO_LE)||(Op == BO_GE)))
+      reportBug((  "\'"
+                 + B->getOpcodeStr().str()
+                 + "\' compare of equal expressions, "
+                     "always evaluates to true").c_str(),
+                 C.getState(), C);
+    else
+      reportBug((  "\'"
+                 + B->getOpcodeStr().str()
+                 + "\' compare of equal expressions, "
+                     "always evaluates to false").c_str(),
+                 C.getState(), C);

I think it's okay to leave the comparison operator out of the message here. "Comparison of identical expressions always evaluates to {true,false}" is fine; if the report is emitted at the BinaryOperator itself, it will get attached to the operator loc.

One last question: what's the performance implication of this check? You might want to wait until you've rewritten it as a syntactic checker to test this, but I'm concerned about the cost of walking and comparing all expressions. The short-circuiting logic in here is very good, though, so maybe it's not an issue.

Thanks, Hans (and Nico), for reviewing this. Thanks, Per, for working on it!
Jordan


On Sep 20, 2013, at 3:06 , Per Viberg <Per.Viberg at evidente.se> wrote:

> 
> Hello,
> 
> please review attached 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.diff
Type: text/x-patch
Size: 30733 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130925/d3595c7c/attachment.bin>
-------------- next part --------------
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits