<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Okay, the checker is committed in r194236. Thanks, Per!</div><div><br></div><div>Jordan</div><div><br></div><br><div><div>On Nov 5, 2013, at 9:39 , Anna Zaks <<a href="mailto:ganna@apple.com">ganna@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Nov 5, 2013, at 9:26 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><base href="x-msg://6219/"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">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? </div></blockquote><div><br></div><div>I don't think it's fair to say that if the reports do not show in clang/llvm codebase, the checker catches problems that don't show up in real world. (Jordan probably misspoke.)</div><div><br></div><div>This type of errors would probably be more common in newly written, not very well tested code. Also, clang/llvm has been checked with other tools, which might have exposed these types of bugs already.</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Anna?</div><div><br></div><div>(The answer is probably "yes", but I thought I might as well bring it up. Perhaps it's disabled in our "shallow" analysis mode.)</div></div></blockquote><div><br></div><div>I think this checker SHOULD be enabled in shallow mode. First, shallow is mostly used when people want fast turnaround (as in running analyzer during build). So there is a high chance the analyzer will be run on freshly written code that, I think, is more likely to have bugs like these. Also, this is a syntactic check; those are almost always fast enough for the analyzer.</div><div><br></div><div>Anna.</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Jordan</div><div><br></div><br><div><div>On Oct 31, 2013, at 3:20 , Per Viberg <<a href="mailto:Per.Viberg@evidente.se">Per.Viberg@evidente.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; direction: ltr; font-family: Tahoma; font-size: 10pt; "><div style="font-family: 'Times New Roman'; font-size: 16px; "><font face="Tahoma" size="2"><br class="Apple-interchange-newline">Hi Jordan,<span class="Apple-converted-space"> </span><br></font><div><div style="direction: ltr; font-family: Tahoma; 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<span class="Apple-converted-space"> </span></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; "><div style="margin-top: 0px; margin-bottom: 0px; "><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray; ">.......................................................................................................................</span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; "><br>Per Viberg<span class="Apple-converted-space"> </span></span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray; ">Senior Engineer</span><span lang="EN-US" style="font-size: 8.5pt; font-family: Arial, sans-serif; color: gray; "><br>Evidente ES East</span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray; "><span class="Apple-converted-space"> </span>AB Warfvinges väg 34 SE-112 51 Stockholm Sweden</span><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif; "></span></div><div style="margin-top: 0px; margin-bottom: 0px; "><span lang="EN-GB" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray; ">Phone: +46 (0)8 402 79 00<br>Mobile: +46 (0)70 912 42 52<br>E-mail: <span class="Apple-converted-space"> </span><a href="mailto:Per.Viberg@evidente.se" target="_blank"><font color="#0000ff">Per.Viberg@evidente.se</font></a><span class="Apple-converted-space"> </span></span><span lang="EN-GB" style="font-size: 8pt; font-family: Arial, sans-serif; "><br><br><a href="http://www.evidente.se/" target="_blank"><font color="#0000ff">www.evidente.se</font></a></span></div><div style="margin-top: 0px; margin-bottom: 0px; "><span lang="EN-GB" style="font-size: 6pt; font-family: Arial, sans-serif; ">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></div></div></div><div style="font-family: 'Times New Roman'; font-size: 16px; "><hr tabindex="-1"><div id="divRpF643227" style="direction: ltr; "><font face="Tahoma" size="2"><b>Från:</b><span class="Apple-converted-space"> </span>Jordan Rose [<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>]<br><b>Skickat:</b><span class="Apple-converted-space"> </span>den 11 oktober 2013 02:43<br><b>Till:</b><span class="Apple-converted-space"> </span>Per Viberg<br><b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><b>Ämne:</b><span class="Apple-converted-space"> </span>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; ">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 <<a href="mailto:Per.Viberg@evidente.se">Per.Viberg@evidente.se</a>> 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: <a href="mailto:Per.Viberg@evidente.se">Per.Viberg@evidente.se</a><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></span></font></div><div class="BodyFragment"><font size="2"><span style="font-size: 10pt; ">> _______________________________________________<br>> cfe-commits mailing list<br>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><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></span></font></div></div></div></div></div></div></div></blockquote></div></div><span><IdenticalExpr_rev3.diff></span><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><base href="x-msg://6219/"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><span style="font-family: Helvetica; font-size: inherit; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">_______________________________________________</span><br style="font-family: Helvetica; font-size: inherit; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: inherit; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">cfe-commits mailing list</span><br style="font-family: Helvetica; font-size: inherit; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><a href="mailto:cfe-commits@cs.uiuc.edu" style="font-family: Helvetica; font-size: inherit; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">cfe-commits@cs.uiuc.edu</a><br style="font-family: Helvetica; font-size: inherit; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" style="font-family: Helvetica; font-size: inherit; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></blockquote></div><br></div></blockquote></div><br></div></blockquote></div><br></body></html>