<html dir="ltr">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<style id="owaParaStyle" type="text/css">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
P {margin-top:0;margin-bottom:0;}</style>
</head>
<body ocsi="0" fpstyle="1">
<div style="direction: ltr;font-family: Tahoma;color: #000000;font-size: 10pt;"><font size="2"><br>
</font>
<div style="font-family: Times New Roman; color: #000000; font-size: 16px"><font face="Tahoma" size="2"></font><font face="Tahoma" size="2">Hi Jordan,
<br>
</font>
<div>
<div style="direction:ltr; font-family:Tahoma; color:#000000; font-size:10pt"><font size="2"><br>
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
</font>analysis on an open source project called cppchecker to check performance.<br>
<br>
Without IdenticalExpChecker: 5197 seconds<br>
With IdenticalExpChecker: 5239 seconds.<br>
Result: approximately 40 seconds longer, which amounts to < 1% increase in analyse time.<br>
<br>
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.<br>
<br>
The checker didn't find any "real" errors in the cppChecker project, and not any false positives.<font size="2"><span style="font-size:10pt"><br>
<br>
</span></font>
<div>best regards,<br>
Per<br>
<div style="font-size:13px; font-family:Tahoma">
<p class="MsoNormal"><span style="font-size:8pt; font-family:'Arial','sans-serif'; color:gray" lang="EN-US">.......................................................................................................................</span><span style="font-size:8pt; font-family:'Arial','sans-serif'; color:black" lang="EN-US"><br>
Per Viberg </span><span style="font-size:8pt; font-family:'Arial','sans-serif'; color:gray" lang="EN-US">Senior Engineer</span><span style="font-size:8.5pt; font-family:'Arial','sans-serif'; color:gray" lang="EN-US"><br>
Evidente ES East</span><span style="font-size:8pt; font-family:'Arial','sans-serif'; color:gray" lang="EN-US"> AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
</span><span style="font-size:10pt; font-family:'Tahoma','sans-serif'; color:black" lang="EN-US"></span></p>
<p class="MsoNormal"><span style="font-size:8pt; font-family:'Arial','sans-serif'; color:gray" lang="EN-GB">Phone: +46 (0)8 402 79 00<br>
Mobile: +46 (0)70 912 42 52<br>
E-mail: <a href="mailto:Per.Viberg@evidente.se" target="_blank"><font color="#0000ff">Per.Viberg@evidente.se</font></a>
</span><span style="font-size:8pt; font-family:'Arial','sans-serif'; color:black" lang="EN-GB"><br>
<br>
<a href="http://www.evidente.se" target="_blank"><font color="#0000ff">www.evidente.se</font></a></span></p>
<p class="MsoNormal"><span style="font-size:6pt; font-family:'Arial','sans-serif'" lang="EN-GB">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.</span></p>
</div>
</div>
<div style="font-family:Times New Roman; color:#000000; font-size:16px">
<hr tabindex="-1">
<div id="divRpF643227" style="direction:ltr"><font color="#000000" face="Tahoma" size="2"><b>Från:</b> Jordan Rose [jordan_rose@apple.com]<br>
<b>Skickat:</b> den 11 oktober 2013 02:43<br>
<b>Till:</b> Per Viberg<br>
<b>Cc:</b> cfe-commits@cs.uiuc.edu<br>
<b>Ämne:</b> Re: [PATCH][StaticAnalyzer] new check comparing equal expression<br>
</font><br>
</div>
<div></div>
<div>
<div class="BodyFragment"><font size="2"><span style="font-size:10pt">
<div class="PlainText">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.)<br>
<br>
<br>
+static bool isSameExpr(const ASTContext &Ctx, Expr *Expr1, Expr *Expr2);<br>
<br>
const Expr *, please?<br>
<br>
<br>
+ // only visit nodes that are binary expressions<br>
<br>
LLVM convention says that all comments should be complete sentences, including capitalization and a final period.<br>
<br>
<br>
+ bool isSameExpression;<br>
<br>
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.<br>
<br>
<br>
+ DeclRefExpr *declref1 = dyn_cast<DeclRefExpr>(LHS);<br>
+ DeclRefExpr *declref2 = dyn_cast<DeclRefExpr>(RHS);<br>
<br>
LLVM convention says that all variable names are UpperCamelCased. There are some of these in isSameExpr as well.<br>
<br>
<br>
+ // If any side of comparison still has floatingpointrepresentation,<br>
+ // then it's an expression-> don't warn.<br>
+ // (here only LHS is checked since RHS will be implicit casted to float)<br>
<br>
Please turn these into complete sentences (and split up "floating-point representation").<br>
<br>
<br>
+ } else { // No special case with floating point representation, report as<br>
+ // usual<br>
+ }<br>
<br>
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.<br>
<br>
<br>
+// t*(u + t) and t*u + t*t are not considered equal.<br>
<br>
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 ///).<br>
<br>
<br>
Has this checker found any real errors in code you have access to? Has it found any false positives?<br>
<br>
Thanks for working on this!<br>
Jordan<br>
<br>
<br>
On Oct 10, 2013, at 8:42 , Per Viberg <Per.Viberg@evidente.se> wrote:<br>
<br>
> <br>
> Hello,<br>
> <br>
> changed patch according to review comments.<br>
> <br>
> Please review revised patch. <br>
> <br>
> The patch adds a check that warns for comparison of equal expressions.<br>
> example:<br>
> x + 1 == x +1;<br>
> <br>
> best regards,<br>
> Per<br>
> <br>
> .......................................................................................................................<br>
> Per Viberg Senior Engineer<br>
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br>
> Phone: +46 (0)8 402 79 00<br>
> Mobile: +46 (0)70 912 42 52<br>
> E-mail: Per.Viberg@evidente.se<br>
> <br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br>
> 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.<br>
</div>
</span></font></div>
<div class="BodyFragment"><font size="2"><span style="font-size:10pt">
<div class="PlainText">> _______________________________________________<br>
> cfe-commits mailing list<br>
> cfe-commits@cs.uiuc.edu<br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</div>
</span></font></div>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>