<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 11.03.2014 20:58, Jordan Rose wrote:<br>
</div>
<blockquote
cite="mid:F3431C39-F218-4211-B8CD-AA7E4D0291E9@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<br>
<div>
<div>On Mar 6, 2014, at 13:16 , Daniel Fahlgren <<a
moz-do-not-send="true" href="mailto:daniel@fahlgren.se">daniel@fahlgren.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: auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-stroke-width: 0px;">Hi
Jordan,<br>
<br>
Thanks for reviewing this.<br>
<br>
On Wed, 2014-03-05 at 14:38 -0800, Jordan Rose wrote:<br>
<blockquote type="cite">I'm sorry, I managed to miss this
one. Please feel free to include me<br>
in the To or CC fields when you have analyzer patches.<br>
</blockquote>
<br>
<blockquote type="cite">I'm glad to get this check! It
definitely seems useful for copy-paste<br>
checking. Thanks for doing the timing checks in advance.<br>
<br>
Comments:<br>
<br>
+ if (Stmt1 && Stmt2) {<br>
+ const Expr *Cond1 = I->getCond();<br>
+ const Stmt *Else = Stmt2;<br>
+ while (const IfStmt *I2 =
dyn_cast<IfStmt>(Else)) {<br>
<br>
If you use dyn_cast_or_null here, then you can drop the
check for a<br>
missing 'else' at the end of the loop (and at the
beginning of the loop<br>
too, though that's less interesting).<br>
</blockquote>
<br>
I've kept the check at the beginning, mostly because I think
it makes<br>
the code more readable.<br>
<br>
<blockquote type="cite">+ PathDiagnosticLocation ELoc
= PathDiagnosticLocation::createBegin(Cond2,<br>
+ BR.getSourceManager(), AC);<br>
<br>
Please use the regular Stmt-based constructor for<br>
PathDiagnosticLocation. We want to consider the whole
condition to be<br>
the location, not just the beginning.<br>
<br>
+ BR.EmitBasicReport(AC->getDecl(), Checker,<br>
+ "Identical conditions",<br>
+ categories::LogicError,<br>
+ "identical condition as a
previous one", ELoc, Sr);<br>
<br>
This isn't quite valid English: two things can "be
identical", and one<br>
thing can "be identical to" another thing, but one thing
can't be<br>
"identical as" another thing. The "one" also makes me<br>
uncomfortable...it feels weird in a compiler tool. How
about<br>
"expression is identical to previous condition"? (I'm
cheating by using<br>
"expression" to mean "condition" in this case, but it
sounds better to<br>
not repeat "condition".)<br>
<br>
As far as test cases go, please include some cases where<br>
- the duplicated condition is not last<br>
- the duplicated condition is not first<br>
- there are multiple pairs of duplicated conditions in the
chain<br>
</blockquote>
<br>
Valid points as usual. Attached is an updated version of the
patch.<br>
<br>
Btw, who should I poke to get the list of potential checkers
updated?<br>
The page still lists different.IdenticalExprBinOp,<br>
different.IdenticalStmtThenElse and
different.CondOpIdenticalReturn as<br>
without progress.<br>
</div>
</blockquote>
</div>
<br>
<div>Ah, that's also me, probably. The original list was set up by
Anna and Anton Yartsev, for the most part, but Anna is still out
and I don't think Anton wants to be the general maintainer for
it. The fastest way to get it updated is to send me a diff; the
entire analyzer site is in www/analyzer/.</div>
<div><br>
</div>
<div>Patch committed as r203585!</div>
<div><br>
</div>
<div>Jordan</div>
</blockquote>
<br>
Hm, the list is really long outdated. I'll update it one of these
days.<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>