<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Dec 11, 2013 at 3:59 AM, Anders Rönnholm <span dir="ltr"><<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div lang="SV" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Hi,<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">I haven’t received any comments on this patch. Is anyone reviewing it?<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">//Anders</span><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<div>
<div style="border-style:solid none none;border-top-color:rgb(181,196,223);border-top-width:1pt;padding:3pt 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif">From:</span></b><span lang="EN-US" style="font-size:10pt;font-family:Tahoma,sans-serif"> Anders Rönnholm
<br>
<b>Sent:</b> den 21 november 2013 08:52<br>
<b>To:</b> 'Anna Zaks'; Richard Trieu<br>
<b>Cc:</b> Jordan Rose; Ted Kremenek; <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> RE: [PATCH] check for Incorrect logic in operator<u></u><u></u></span></p>
</div>
</div><div><div>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Hi,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Here is an updated patch I’d like to get reviewed. The warnings have been moved to the unreachable-code group, I hope that’s fine.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Thanks,<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Anders<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> </span></p></div></div></div></div></blockquote><div><br></div><div>I don't like the warnings in the unreachable-code group, since in the always true case, it is the opposite of unreachable code. However, existing the existing warning groups that this would belong to are on by default, which typically exclude CFG warnings. We may need a temporary solution until we merge the old and new warnings.</div>
<div><br></div><div>Also, does your patch cause the unreachable-code warning to trigger in more cases now?</div><div><br></div><div>More comments inline.</div><div><br></div><div> Index: lib/Analysis/CFG.cpp</div><div>===================================================================</div>
<div>--- lib/Analysis/CFG.cpp<span style="white-space:pre-wrap"> </span>(revision 194562)</div><div>+++ lib/Analysis/CFG.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -483,6 +483,192 @@</div>
<div> B->addSuccessor(S, cfg->getBumpVectorContext());</div><div> }</div><div><br></div><div>+ /// \brief Find a relational comparison with an expression evaluating to a</div><div>+ /// boolean and a constant other than 0 and 1.</div>
<div>+ /// e.g. if ((x < y) == 10)</div><div>+ TryResult checkIncorrectRelationalOperator(const BinaryOperator *B) {</div><div>+ const IntegerLiteral *IntLiteral =</div><div>+ dyn_cast<IntegerLiteral>(B->getLHS()->IgnoreParens());</div>
<div>+ const Expr *BoolExpr = B->getRHS()->IgnoreParens();</div><div>+ bool IntFirst = true;</div><div>+ if (!IntLiteral) {</div><div>+ IntLiteral = dyn_cast<IntegerLiteral>(B->getRHS()->IgnoreParens());</div>
<div>+ BoolExpr = B->getLHS()->IgnoreParens();</div><div>+ IntFirst = false;</div><div>+ }</div><div>+</div><div>+ if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue())</div><div>+ return TryResult();</div>
<div>+</div><div>+ llvm::APInt IntValue = IntLiteral->getValue();</div><div>+ if ((IntValue != 1) && (IntValue != 0)) {</div><div>+ BuildOpts.Observer->compareBoolWithInt(B);</div><div><br></div>
<div>
Check that BuildOpts.Observer is not null before dereferencing.</div><div><br></div><div>+ bool IntLarger = !IntValue.isNegative();</div><div>+ BinaryOperatorKind Bok = B->getOpcode();</div><div>+ if (Bok == BO_GT || Bok == BO_GE) {</div>
<div>+ return TryResult(IntFirst != IntLarger);</div><div>+ } else {</div><div>+ return TryResult(IntFirst == IntLarger);</div><div>+ }</div><div>+ }</div><div>+ return TryResult();</div><div>
+ }</div><div>+</div><div>+ /// Find a equality comparison with an expression evaluating to a boolean and</div><div>+ /// a constant other than 0 and 1.</div><div>+ /// e.g. if (!x == 10)</div><div>+ TryResult checkIncorrectEqualityOperator(const BinaryOperator *B) {</div>
<div>+ const IntegerLiteral *IntLiteral =</div><div>+ dyn_cast<IntegerLiteral>(B->getLHS()->IgnoreParens());</div><div>+ const Expr *BoolExpr = B->getRHS()->IgnoreParens();</div><div>+</div>
<div>+ if (!IntLiteral) {</div><div>+ IntLiteral = dyn_cast<IntegerLiteral>(B->getRHS()->IgnoreParens());</div><div>+ BoolExpr = B->getLHS()->IgnoreParens();</div><div>+ }</div><div>+</div>
<div>+ if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue())</div><div>+ return TryResult();</div><div>+</div><div>+ llvm::APInt IntValue = IntLiteral->getValue();</div><div>+ if ((IntValue != 1) && (IntValue != 0)) {</div>
<div>+ BuildOpts.Observer->compareBoolWithInt(B);</div><div>+ return TryResult(B->getOpcode() != BO_EQ);</div><div>+ }</div><div>+ return TryResult();</div><div>+ }</div><div>+</div><div>+ bool analyzeLogicOperatorCondition(BinaryOperatorKind Relation,</div>
<div>+ const llvm::APSInt Value1,</div><div>+ const llvm::APSInt Value2) {</div><div><br></div><div>A switch statement would be nicer here. Also, add an assert that the two values have the same signedness and bit width.</div>
<div><br></div><div>+ return (Relation == BO_EQ && (Value1 == Value2)) ||</div><div>+ (Relation == BO_NE && (Value1 != Value2)) ||</div><div>+ (Relation == BO_LT && (Value1 < Value2)) ||</div>
<div>+ (Relation == BO_LE && (Value1 <= Value2)) ||</div><div>+ (Relation == BO_GT && (Value1 > Value2)) ||</div><div>+ (Relation == BO_GE && (Value1 >= Value2));</div>
<div>+ }</div><div>+</div><div>+ /// \brief Find a pair of comparison expressions with or without parentheses</div><div>+ /// with a shared variable and constants and a logical operator between them</div><div>+ /// that always evaluates to either true or false.</div>
<div>+ /// e.g. if (x != 3 || x != 4)</div><div>+ TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {</div><div>+ const BinaryOperator *LHS =</div><div>+ dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());</div>
<div>+ const BinaryOperator *RHS =</div><div>+ dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());</div><div>+ if (!LHS || !RHS)</div><div>+ return TryResult();</div><div>+</div><div>+ if (!LHS->isComparisonOp() || !RHS->isComparisonOp())</div>
<div>+ return TryResult();</div><div>+</div><div><br></div><div>Instead of looking for an integer literal, see if the expression can be evaluated, and use that as the constant value. This will allow things like 1+2 and -1.</div>
<div><br></div><div>+ BinaryOperatorKind BO1 = LHS->getOpcode();</div><div>+ const DeclRefExpr *Decl1 =</div><div>+ dyn_cast<DeclRefExpr>(LHS->getLHS()->IgnoreParenImpCasts());</div><div>+ const IntegerLiteral *Literal1 =</div>
<div>+ dyn_cast<IntegerLiteral>(LHS->getRHS()->IgnoreParens());</div><div>+</div><div>+ if (!Decl1 && !Literal1) {</div><div>+ if (BO1 == BO_GT)</div><div>+ BO1 = BO_LT;</div><div>
+ else if (BO1 == BO_GE)</div><div>+ BO1 = BO_LE;</div><div>+ else if (BO1 == BO_LT)</div><div>+ BO1 = BO_GT;</div><div>+ else if (BO1 == BO_LE)</div><div>+ BO1 = BO_GE;</div><div>+ Decl1 =</div>
<div>+ dyn_cast<DeclRefExpr>(LHS->getRHS()->IgnoreParenImpCasts());</div><div>+ Literal1 = dyn_cast<IntegerLiteral>(LHS->getLHS()->IgnoreParens());</div><div>+ }</div><div>+</div>
<div>
+ if (!Decl1 || !Literal1)</div><div>+ return TryResult();</div><div>+</div><div>+ BinaryOperatorKind BO2 = RHS->getOpcode();</div><div>+ const DeclRefExpr *Decl2 =</div><div>+ dyn_cast<DeclRefExpr>(RHS->getLHS()->IgnoreImpCasts()->IgnoreParens());</div>
<div><br></div><div>->IgnoreParenImpCasts() instead of ->IgnoreImpCasts()->IgnoreParens()</div><div><br></div><div>+ const IntegerLiteral *Literal2 =</div><div>+ dyn_cast<IntegerLiteral>(RHS->getRHS()->IgnoreParens());</div>
<div>+</div><div>+ if (!Decl2 && !Literal2) {</div><div>+ if (BO2 == BO_GT)</div><div>+ BO2 = BO_LT;</div><div>+ else if (BO2 == BO_GE)</div><div>+ BO2 = BO_LE;</div><div>+ else if (BO2 == BO_LT)</div>
<div>+ BO2 = BO_GT;</div><div>+ else if (BO2 == BO_LE)</div><div>+ BO2 = BO_GE;</div><div>+ Decl2 =</div><div>+ dyn_cast<DeclRefExpr>(RHS->getRHS()->IgnoreImpCasts()->IgnoreParens());</div>
<div>+ Literal2 = dyn_cast<IntegerLiteral>(RHS->getLHS()->IgnoreParens());</div><div>+ }</div><div>+</div><div>+ if (!Decl2 || !Literal2)</div><div>+ return TryResult();</div><div>+</div><div>
+ // Check that it is the same variable on both sides.</div>
<div>+ if (Decl1->getNameInfo().getAsString() != Decl2->getNameInfo().getAsString())</div><div><br></div><div>Decl1->getDecl() != Decl2->getDecl()</div><div><br></div><div>+ return TryResult();</div><div>
+</div><div>+ // evaluate if expression is always true/false</div><div>+ llvm::APSInt L1,L2;</div><div>+ if (!Literal1->EvaluateAsInt(L1,*Context) ||</div><div>+ !Literal2->EvaluateAsInt(L2,*Context))</div>
<div>+ return TryResult();</div><div>+</div><div>+ const llvm::APSInt MinValueL1 =</div><div>+ llvm::APSInt::getMinValue(L1.getBitWidth(),L1.isUnsigned());</div><div>+ const llvm::APSInt MaxValueL1 =</div>
<div>+ llvm::APSInt::getMaxValue(L1.getBitWidth(),L1.isUnsigned());</div><div>+</div><div>+ const llvm::APSInt MinValueL2 =</div><div>+ llvm::APSInt::getMinValue(L2.getBitWidth(),L2.isUnsigned());</div><div>
+ const llvm::APSInt MaxValueL2 =</div><div>+ llvm::APSInt::getMaxValue(L2.getBitWidth(),L2.isUnsigned());</div><div>+ bool AlwaysTrue = true, AlwaysFalse = true;</div><div><br></div><div>What is going on here?</div>
<div>+ for (int Offset = -3; Offset <=3; ++Offset) {</div><div>+ for (int Selvalue = 1; Selvalue <= 2; Selvalue++) {</div><div>+</div><div>+ llvm::APSInt SelInt = (Selvalue==1 ? L1 : L2);</div><div>+ llvm::APSInt SelMin = (Selvalue==1 ? MinValueL1 : MinValueL2);</div>
<div>+ llvm::APSInt SelMax = (Selvalue==1 ? MaxValueL1 : MaxValueL2);</div><div>+</div><div>+ llvm::APSInt OffsetAPS =</div><div>+ llvm::APSInt(llvm::APInt(SelInt.getBitWidth(), Offset),</div><div>
+ SelInt.isUnsigned());</div><div>+</div><div>+ if ((SelInt + OffsetAPS) > SelMax)</div><div>+ continue;</div><div>+ else if ((SelInt + OffsetAPS) < SelMin)</div><div>
+ continue;</div>
<div>+ else</div><div>+ SelInt += OffsetAPS;</div><div>+</div><div>+ bool Res1,Res2;</div><div>+ Res1 = analyzeLogicOperatorCondition(BO1,SelInt, L1);</div><div>+ Res2 = analyzeLogicOperatorCondition(BO2,SelInt, L2);</div>
<div>+ if (B->getOpcode() == BO_LAnd) {</div><div>+ AlwaysTrue &= (Res1 && Res2);</div><div>+ AlwaysFalse &= !(Res1 && Res2);</div><div>+ } else {</div><div>
+ AlwaysTrue &= (Res1 || Res2);</div><div>+ AlwaysFalse &= !(Res1 || Res2);</div><div>+ }</div><div>+ }</div><div>+ }</div><div>+</div><div>+ if(AlwaysTrue || AlwaysFalse) {</div>
<div>+ BuildOpts.Observer->compareAlwaysTrue(B,AlwaysTrue);</div><div>+ return TryResult(AlwaysTrue);</div><div>+ }</div><div>+</div><div>+ return TryResult();</div><div>+ }</div><div>+</div><div> /// Try and evaluate an expression to an integer constant.</div>
<div> bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) {</div><div> if (!BuildOpts.PruneTriviallyFalseEdges)</div><div>@@ -565,13 +751,24 @@</div><div> // is determined by the RHS: X && 0 -> 0, X || 1 -> 1.</div>
<div> if (RHS.isTrue() == (Bop->getOpcode() == BO_LOr))</div><div> return RHS.isTrue();</div><div>+ } else {</div><div>+ TryResult BopRes = checkIncorrectLogicOperator(Bop);</div>
<div>+ if (BopRes.isKnown())</div><div>+ return BopRes.isTrue();</div><div> }</div><div> }</div><div><br></div><div> return TryResult();</div><div>+ } else if (Bop->isEqualityOp()) {</div>
<div>+ TryResult BopRes = checkIncorrectEqualityOperator(Bop);</div><div>+ if (BopRes.isKnown())</div><div>+ return BopRes.isTrue();</div><div>+ } else if (Bop->isRelationalOp()) {</div>
<div>+ TryResult BopRes = checkIncorrectRelationalOperator(Bop);</div><div>+ if (BopRes.isKnown())</div><div>+ return BopRes.isTrue();</div><div> }</div><div> }</div><div>-</div><div> bool Result;</div>
<div> if (E->EvaluateAsBooleanCondition(Result, *Context))</div><div> return Result;</div><div>@@ -1311,6 +1508,8 @@</div><div> else {</div><div> RHSBlock->setTerminator(Term);</div><div> TryResult KnownVal = tryEvaluateBool(RHS);</div>
<div>+ if (!KnownVal.isKnown())</div><div>+ KnownVal = tryEvaluateBool(B);</div><div> addSuccessor(RHSBlock, KnownVal.isFalse() ? NULL : TrueBlock);</div><div> addSuccessor(RHSBlock, KnownVal.isTrue() ? NULL : FalseBlock);</div>
<div> }</div><div>Index: lib/Sema/AnalysisBasedWarnings.cpp</div><div>===================================================================</div><div>--- lib/Sema/AnalysisBasedWarnings.cpp<span style="white-space:pre-wrap"> </span>(revision 194562)</div>
<div>+++ lib/Sema/AnalysisBasedWarnings.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -77,6 +77,60 @@</div><div> reachable_code::FindUnreachableCode(AC, UC);</div><div>}</div><div><br></div>
<div>+/// \brief Warn on logical operator errors in CFGBuilder</div><div>+class LogicalErrorsHandler : public CFGCallback {</div><div>+ Sema &S;</div><div>+public:</div><div>+ LogicalErrorsHandler(Sema &S) : CFGCallback(),S(S) {}</div>
<div>+ void compareAlwaysTrue(const BinaryOperator* B, bool isAlwaysTrue) {</div><div>+ if (B->getLHS()->getExprLoc().isMacroID() ||</div><div>+ B->getRHS()->getExprLoc().isMacroID())</div><div>+ return;</div>
<div>+</div><div>+ const BinaryOperator *LHS =</div><div>+ dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());</div><div>+ const BinaryOperator *RHS =</div><div>+ dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());</div>
<div>+</div><div>+ if (LHS)</div><div>+ if (LHS->getLHS()->getExprLoc().isMacroID() ||</div><div>+ LHS->getRHS()->getExprLoc().isMacroID())</div><div>+ return;</div><div>+ if (RHS)</div>
<div>+ if (RHS->getLHS()->getExprLoc().isMacroID() ||</div><div>+ RHS->getRHS()->getExprLoc().isMacroID())</div><div>+ return;</div><div>+</div><div>+ SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()->getLocEnd());</div>
<div>+ S.Diag(B->getExprLoc(), diag::warn_operator_always_true_comparison)</div><div>+ << DiagRange << isAlwaysTrue;</div><div>+ }</div><div>+</div><div>+ void compareBoolWithInt(const BinaryOperator* B) {</div>
<div>+ if (B->getLHS()->getExprLoc().isMacroID() ||</div><div>+ B->getRHS()->getExprLoc().isMacroID())</div><div>+ return;</div><div>+</div><div>+ const BinaryOperator *LHS =</div><div>+ dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());</div>
<div>+ const BinaryOperator *RHS =</div><div>+ dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());</div><div>+</div><div>+ if (LHS)</div><div>+ if (LHS->getLHS()->getExprLoc().isMacroID() ||</div>
<div>+ LHS->getRHS()->getExprLoc().isMacroID())</div><div>+ return;</div><div>+ if (RHS)</div><div>+ if (RHS->getLHS()->getExprLoc().isMacroID() ||</div><div>+ RHS->getRHS()->getExprLoc().isMacroID())</div>
<div>+ return;</div><div>+</div><div>+ SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()->getLocEnd());</div><div>+ S.Diag(B->getExprLoc(), diag::warn_bool_and_int_comparison) << DiagRange;</div>
<div>+ }</div><div>+};</div><div>+</div><div>+</div><div>//===----------------------------------------------------------------------===//</div><div>// Check for missing return value.</div><div>//===----------------------------------------------------------------------===//</div>
<div>@@ -1723,8 +1777,11 @@</div><div> bool isTemplateInstantiation = false;</div><div> if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D))</div><div> isTemplateInstantiation = Function->isTemplateInstantiation();</div>
<div>- if (!isTemplateInstantiation)</div><div>+ if (!isTemplateInstantiation) {</div><div>+ LogicalErrorsHandler Leh(S);</div><div>+ AC.getCFGBuildOptions().Observer = &Leh;</div><div> CheckUnreachable(S, AC);</div>
<div>+ }</div><div> }</div><div><br></div><div> // Check for thread safety violations</div><div>Index: lib/AST/Expr.cpp</div><div>===================================================================</div><div>--- lib/AST/Expr.cpp<span style="white-space:pre-wrap"> </span>(revision 194562)</div>
<div>+++ lib/AST/Expr.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -152,6 +152,8 @@</div><div> switch (UO->getOpcode()) {</div><div> case UO_Plus:</div><div> return UO->getSubExpr()->isKnownToHaveBooleanValue();</div>
<div>+ case UO_LNot:</div><div>+ return true;</div><div> default:</div><div> return false;</div><div> }</div><div>Index: test/Sema/warn-overlap.c</div><div>===================================================================</div>
<div>--- test/Sema/warn-overlap.c<span style="white-space:pre-wrap"> </span>(revision 0)</div><div>+++ test/Sema/warn-overlap.c<span style="white-space:pre-wrap"> </span>(working copy)</div><div><br></div><div>Add some tests with different variable types and different constant types.</div>
<div><br></div><div>@@ -0,0 +1,51 @@</div>
<div>+// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s</div><div>+</div><div>+#define mydefine 2</div><div>+</div><div>+void f(int x) {</div><div>+ int y = 0;</div><div>+ // > || <</div><div>+ if (x > 2 || x < 1) { }</div>
<div>+ if (x > 2 || x < 2) { }</div><div>+ if (x != 2 || x != 3) { } // expected-warning {{logical disjunction always evaluates to true}}</div><div>+ if (x > 2 || x < 3) { } // expected-warning {{logical disjunction always evaluates to true}}</div>
<div>+</div><div>+ if (x > 2 || x <= 1) { }</div><div>+ if (x > 2 || x <= 2) { } // expected-warning {{logical disjunction always evaluates to true}}</div><div>+ if (x > 2 || x <= 3) { } // expected-warning {{logical disjunction always evaluates to true}}</div>
<div>+</div><div>+ if (x >= 2 || x < 1) { }</div><div>+ if (x >= 2 || x < 2) { } // expected-warning {{logical disjunction always evaluates to true}}</div><div>+ if (x >= 2 || x < 3) { } // expected-warning {{logical disjunction always evaluates to true}}</div>
<div>+</div><div>+ if (x >= 2 || x <= 1) { } // expected-warning {{logical disjunction always evaluates to true}}</div><div>+ if (x >= 2 || x <= 2) { } // expected-warning {{logical disjunction always evaluates to true}}</div>
<div>+ if (x >= 2 || x <= 3) { } // expected-warning {{logical disjunction always evaluates to true}}</div><div>+</div><div>+ // > && <</div><div>+ if (x > 2 && x < 1) { } // expected-warning {{logical disjunction always evaluates to false}}</div>
<div>+ if (x > 2 && x < 2) { } // expected-warning {{logical disjunction always evaluates to false}}</div><div>+ if (x > 2 && x < 3) { } // expected-warning {{logical disjunction always evaluates to false}}</div>
<div>+</div><div>+ if (x > 2 && x <= 1) { } // expected-warning {{logical disjunction always evaluates to false}}</div><div>+ if (x > 2 && x <= 2) { } // expected-warning {{logical disjunction always evaluates to false}}</div>
<div>+ if (x > 2 && x <= 3) { }</div><div>+</div><div>+ if (x >= 2 && x < 1) { } // expected-warning {{logical disjunction always evaluates to false}}</div><div>+ if (x >= 2 && x < 2) { } // expected-warning {{logical disjunction always evaluates to false}}</div>
<div>+ if (x >= 2 && x < 3) { }</div><div>+</div><div>+ if (x >= 2 && x <= 1) { } // expected-warning {{logical disjunction always evaluates to false}}</div><div>+ if (x >= 2 && x <= 2) { }</div>
<div>+ if (x >= 2 && x <= 3) { }</div><div>+</div><div>+ // !=, ==, ..</div><div>+ if (x != 2 || x != 3) { } // expected-warning {{logical disjunction always evaluates to true}}</div><div>+ if (x != 2 || x < 3) { } // expected-warning {{logical disjunction always evaluates to true}}</div>
<div>+ if (x == 2 && x == 3) { } // expected-warning {{logical disjunction always evaluates to false}}</div><div>+ if (x == 2 && x > 3) { } // expected-warning {{logical disjunction always evaluates to false}}</div>
<div>+</div><div>+ if (x == mydefine && x > 3) { } // no-warning</div><div>+ if (x == 2 && y > 3) { } // no-warning</div><div>+}</div><div>+</div><div>Index: test/Sema/warn-compint.c</div><div>===================================================================</div>
<div>--- test/Sema/warn-compint.c<span style="white-space:pre-wrap"> </span>(revision 0)</div><div>+++ test/Sema/warn-compint.c<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -0,0 +1,39 @@</div>
<div>+// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s</div><div>+</div><div>+#define mydefine 2</div><div>+</div><div>+void err(int x, int y) {</div><div>+</div><div>+ if (!x == 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div>
<div>+ if (!x != 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div><div>+ if (!x < 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div>
<div>+ if (!x > 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div><div>+ if (!x >= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div>
<div>+ if (!x <= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div><div>+</div><div>+ if ((x < y) == 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div>
<div>+ if ((x < y) != 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div><div>+ if ((x < y) < 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div>
<div>+ if ((x < y) > 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div><div>+ if ((x < y) >= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div>
<div>+ if ((x < y) <= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}}</div><div>+}</div><div>+</div><div>+void noerr(int x, int y) {</div><div>+</div><div>+ if (!x == 1) {}</div>
<div>+ if (!x != 0) {}</div><div>+ if (!x < 1) {}</div><div>+ if (!x > 0) {}</div><div>+ if (!x >= 1) {}</div><div>+ if (!x <= 0) {}</div><div>+</div><div>+ if ((x < y) == 1) {}</div><div>
+ if ((x < y) != 0) {}</div><div>+ if ((x < y) < 1) {}</div><div>+ if ((x < y) > 0) {}</div><div>+ if ((x < y) >= 1) {}</div><div>+ if ((x < y) <= 0) {}</div><div>+</div><div>+ if ((x < mydefine) <= 10) {}</div>
<div>+} // no-warning</div><div>Index: include/clang/Basic/DiagnosticSemaKinds.td</div><div>===================================================================</div><div>--- include/clang/Basic/DiagnosticSemaKinds.td<span style="white-space:pre-wrap"> </span>(revision 194562)</div>
<div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -339,6 +339,12 @@</div><div> InGroup<MissingNoreturn>, DefaultIgnore;</div><div>def warn_unreachable : Warning<"will never be executed">,</div>
<div> InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;</div><div><br></div><div>Still don't like the logical disjunction text. I think it would be confusing for users.</div><div><br></div>
<div>+def warn_operator_always_true_comparison : Warning<</div><div>+ "logical disjunction always evaluates to %select{false|true}0">,</div><div>+ InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;</div>
<div>+def warn_bool_and_int_comparison : Warning<</div><div>+ "comparison of a boolean expression with an integer other than 0 or 1">,</div><div>+ InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;</div>
<div><br></div><div>/// Built-in functions.</div><div>def ext_implicit_lib_function_decl : ExtWarn<</div><div>Index: include/clang/Analysis/CFG.h</div><div>===================================================================</div>
<div>--- include/clang/Analysis/CFG.h<span style="white-space:pre-wrap"> </span>(revision 194562)</div><div>+++ include/clang/Analysis/CFG.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -45,6 +45,7 @@</div>
<div> class ASTContext;</div><div> class CXXRecordDecl;</div><div> class CXXDeleteExpr;</div><div>+ class BinaryOperator;</div><div><br></div><div>/// CFGElement - Represents a top-level expression in a basic block.</div>
<div>class CFGElement {</div><div>@@ -609,6 +610,16 @@</div><div> }</div><div>};</div><div><br></div><div>What's the plan with CFGCallBack? New methods added as needed for each new warning? Does the CFGBuilder need to support multiple CFGCallBack's at once?</div>
<div><br></div><div>+/// \brief CFGCallback defines methods that should be called when a logical</div><div>+/// operator error is found when building the CFG.</div><div>+class CFGCallback {</div><div>+public:</div><div>+ CFGCallback(){}</div>
<div>+ virtual void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {};</div><div>+ virtual void compareBoolWithInt(const BinaryOperator *B) {};</div><div>+ virtual ~CFGCallback(){}</div><div>+};</div><div>
+</div><div>/// CFG - Represents a source-level, intra-procedural CFG that represents the</div><div>/// control-flow of a Stmt. The Stmt can represent an entire function body,</div><div>/// or a single expression. A CFG will always contain one empty block that</div>
<div>@@ -627,7 +638,7 @@</div><div> public:</div><div> typedef llvm::DenseMap<const Stmt *, const CFGBlock*> ForcedBlkExprs;</div><div> ForcedBlkExprs **forcedBlkExprs;</div><div>-</div><div>+ CFGCallback* Observer;</div>
<div> bool PruneTriviallyFalseEdges;</div><div> bool AddEHEdges;</div><div> bool AddInitializers;</div><div>@@ -650,7 +661,7 @@</div><div> }</div><div><br></div><div> BuildOptions()</div><div>- : forcedBlkExprs(0), PruneTriviallyFalseEdges(true)</div>
<div>+ : forcedBlkExprs(0), Observer(0), PruneTriviallyFalseEdges(true)</div><div> ,AddEHEdges(false)</div><div> ,AddInitializers(false)</div><div> ,AddImplicitDtors(false)</div></div></div></div>