<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Mar 6, 2014, at 13:16 , Daniel Fahlgren <<a 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></body></html>