<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Looking good! But I still think we should just highlight the true and false expressions, not the entire conditional (which may span multiple lines):</div><div><br></div><div><div>+ BR.EmitBasicReport(</div><div>+ AC->getDecl(), "Identical expressions in conditional expression",</div><div>+ categories::LogicError,</div><div>+ "identical expressions on both sides of ':' in conditional expression",</div><div>+ ELoc,C->getSourceRange());</div></div><div><br></div><div>You can simply construct a SourceRange[] on the stack and then pass that to the ArrayRef<SourceRange> argument.</div><div><br></div><div>More comments:</div><div><br></div><div><div>+ static PathDiagnosticLocation createConditionalColonLoc(</div><div>+ const ConditionalOperator *CO,</div><div>+ const SourceManager &SM);</div></div><div><br></div><div>I think the new style is to either break after the return type or to double-indent wrapped arguments, rather than right-aligning them. (Right-aligning is harder to maintain.)</div><div><br></div><div><br></div><div><div> static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,</div><div>- const Expr *Expr2);</div><div>+ const Expr *Expr2,</div><div>+ const bool IgnoreSideEffects = false);</div></div><div><br></div><div>We generally don't bother making parameters const. Pointers-to-const, yes, but not the parameters themselves. (It does help catch stupid mistakes sometimes, but it's not worth being inconsistent with the rest of Clang.)</div><div><br></div><div><br></div><div><div>+ if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),</div><div>+ C->getFalseExpr(),true)) {</div></div><div><br></div><div>Missing space after the comma.</div><div><br></div><div><br></div><div><div>- if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2))</div><div>+ if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,</div><div>+ IgnoreSideEffects))</div></div><div><br></div><div>Indentation is wrong; the 'IgnoreSideEffects' is an argument to 'isIdenticalExpr', so it should be aligned with the first argument in the call. (I misread this at first as another || possibility.</div><div><br></div><div><br></div><div><div> case Stmt::ArraySubscriptExprClass:</div><div>+ case Stmt::CallExprClass:</div><div> case Stmt::CStyleCastExprClass:</div><div> case Stmt::ImplicitCastExprClass:</div><div> case Stmt::ParenExprClass:</div></div><div><br></div><div>Aren't calls only identical if IgnoreSideEffects is true? (Or if the callee is constexpr or __attribute__((pure)) or ((const)), I suppose, but I'm okay with leaving that out.)</div><div><br></div><div>You could argue that calls being identical on both sides of an operator might still be an error because the calls are unsequenced, but I suppose that doesn't matter if you're using == or !=. So I don't think we can treat them as identical here, much as I'd like to, unless we have IgnoreSideEffects.</div><div><br></div><div>Please add tests for the comparisons operators involving calls as well.</div><div><br></div><div><br></div><div><div> PathDiagnosticLocation</div><div>+ PathDiagnosticLocation::createConditionalColonLoc(</div><div>+ const ConditionalOperator *CO,</div><div>+ const SourceManager &SM) {</div><div>+ return PathDiagnosticLocation(CO->getColonLoc(), SM, SingleLocK);</div><div>+}</div></div><div><br></div><div>Similar to above: prefer double-indented parameters to right-aligned parameters, and no need for that first indent before the qualified name. (I see other functions in the file are doing that, but not new ones.)</div><div><br></div><div><br></div><div><div>+void test_expr_negative_func() {</div><div>+ unsigned a = 0;</div><div>+ unsigned b = 1;</div><div>+ a = a > 5 ? a+func() : a+func2(); // no warning</div><div>+}</div></div><div><br></div><div>Please also include a test where you're calling the same function with different arguments, to demonstrate that we're stepping into the CallExpr.</div><div><br></div><div>Thanks!</div><div>Jordan</div><div><br></div><br><div><div>On Nov 27, 2013, at 4:43 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div lang="SV" link="blue" vlink="purple" style="font-family: Helvetica; font-size: 12px; 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;"><div class="WordSection1" style="page: WordSection1;"><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Hi,<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">I have updated the check to warn on a++,a=1 and a+func() as the comments suggested.<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">/Anders<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">> I think we should actually be warning, since only one side of the branch will ever be executing. Maybe we can include a flag here that says whether the expression walker should step into calls? What do you think?<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">This sounds reasonable. We could warn about other "non-const" expressions too, I believe:<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> a ? b++ : b++;<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> a ? b=2 : b=2;<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Best regards,<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Daniel Marjamäki<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">..................................................................................................................<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Daniel Marjamäki Senior Engineer<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Mobile: +46 (0)709 12 42 62<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">E-mail: Daniel.Marjamaki at<span class="Apple-converted-space"> </span><a href="http://evidente.se/" style="color: purple; text-decoration: underline;">evidente.se</a><o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><a href="http://www.evidente.se/" style="color: purple; text-decoration: underline;">www.evidente.se</a><o:p></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><o:p> </o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">________________________________________<o:p></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">Från: Jordan Rose [jordan_rose at<span class="Apple-converted-space"> </span><a href="http://apple.com/" style="color: purple; text-decoration: underline;">apple.com</a>]<o:p></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">Skickat: den 20 november 2013 18:22<o:p></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Till: Per Viberg<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Cc: cfe-commits at<span class="Apple-converted-space"> </span><a href="http://cs.uiuc.edu/" style="color: purple; text-decoration: underline;">cs.uiuc.edu</a>; Daniel Marjamäki<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Ämne: Re: [PATCH] new check checking use of identical expressions inside a conditional expression, i.e. '?'.<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Hm, interesting. In this case:<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">+void test_expr_negative_func() {<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">+ unsigned a = 0;<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">+ unsigned b = 1;<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">+ a = a > 5 ? a+func() : a+func(); // no warning<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">+}<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">I think we should actually be warning, since only one side of the branch will ever be executing. Maybe we can include a flag here that says whether the expression walker should step into calls? What do you think?<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US">Other than that, this looks good, except that I would add the true-expr and false-expr as ranges to highlight in addition to putting the warning itself on the colon.<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;"><span lang="EN-US"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">Jordan<o:p></o:p></div></div></div></blockquote></div></body></html>