<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Dec 12, 2013 at 7:02 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div class="im">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><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.</div>
</div></div></div></blockquote><div><br></div><div>The tautological comparison group makes more sense for this warning.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> However, existing the existing warning groups that this would belong to are on by default, which typically exclude CFG warnings.</div></div></div></div>
</blockquote><div><br></div><div>We can have some warnings in a group on by default and some off by default, but this does suggest that we'd want a new warning subgroup for this warning.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> 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 class="im"><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><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 class="im"><div>+</div><div>+ SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()->getLocEnd());</div></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 class="im"><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><div class="im"><div>+ if (!isTemplateInstantiation) {</div><div>+ LogicalErrorsHandler Leh(S);</div></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 class="im"><div>+</div><div>+#define mydefine 2</div><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 class="im"><div>+</div><div>+#define mydefine 2</div><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 class="im"><div>
+</div><div>+ if ((x < mydefine) <= 10) {}</div>
<div>+} // no-warning</div></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 class="im">
<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><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>
<br>_______________________________________________<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></blockquote></div><br></div></div>