[PATCH] check for Incorrect logic in operator

Anders Rönnholm Anders.Ronnholm at evidente.se
Fri Feb 21 05:48:18 PST 2014


Pinging this patch. 

> -----Original Message-----
> From: Anders Rönnholm
> Sent: den 20 december 2013 13:35
> To: Richard Smith; Richard Trieu
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [PATCH] check for Incorrect logic in operator
> 
> Tanks for your comments. New patch attached.
> 
> -- 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.
> Do you want a new subgroup and how do you create one? I have moved it
> back to tautological and made a what i think is a subgroup.
> 
> -- 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.
> I will skip this because it will also generate false positives when it's x+1 and
> can be evaluated.
> 
> --Also, does your patch cause the unreachable-code warning to trigger in
> more cases now?
> I have tested it on llvm and the patch did not trigger more unreachable-code
> warnings.
> 
> -- What is going on here?
> Comments in code, also slight refactoring Basicly it checks whether the
> expression is always true/false by checking following scenarios.
>     x is less than the smallest literal.
>     x is equal to the smallest literal.
>     x is between smallest and largest literal.
>     x is equal to the largest literal.
>     x is greater than largest literal.
> 
> For && both RHS and LHS needs to be true or false in every scenario for it to
> be regarded always true/false.
> For || either RHS and LHS needs to be true or false in every scenario for it to
> be regarded always true/false.
> 
> -- 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?
> Yes new methods would have to be added. No i don't see a reason for the
> need of supporting multiple CFGCallBack's at once.
> 
> //Anders
> 
> 
> .....................................................................................................................
> ..
> Anders Rönnholm Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> 
> Mobile:                    +46 (0)70 912 42 54
> E-mail:                    Anders.Ronnholm at evidente.se
> 
> www.evidente.se
> 
> ________________________________________
> Från: metafoo at gmail.com [metafoo at gmail.com] för Richard Smith
> [richard at metafoo.co.uk]
> Skickat: den 13 december 2013 19:18
> Till: Richard Trieu
> Cc: Anders Rönnholm; cfe-commits at cs.uiuc.edu
> Ämne: Re: [PATCH] check for Incorrect logic in operator
> 
> On Thu, Dec 12, 2013 at 7:02 PM, Richard Trieu
> <rtrieu at google.com<mailto:rtrieu at google.com>> wrote:
> 
> 
> 
> On Wed, Dec 11, 2013 at 3:59 AM, Anders Rönnholm
> <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>>
> wrote:
> Hi,
> 
> I haven't received any comments on this patch. Is anyone reviewing it?
> 
> //Anders
> 
> From: Anders Rönnholm
> Sent: den 21 november 2013 08:52
> To: 'Anna Zaks'; Richard Trieu
> Cc: Jordan Rose; Ted Kremenek; cfe-commits at cs.uiuc.edu<mailto:cfe-
> commits at cs.uiuc.edu>
> Subject: RE: [PATCH] check for Incorrect logic in operator
> 
> Hi,
> 
> 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.
> 
> Thanks,
> Anders
> 
> 
> I don't like the warnings in the unreachable-code group, since in the always
> true case, it is the opposite of unreachable code.
> 
> The tautological comparison group makes more sense for this warning.
> 
>  However, existing the existing warning groups that this would belong to are
> on by default, which typically exclude CFG warnings.
> 
> 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.
> 
>  We may need a temporary solution until we merge the old and new
> warnings.
> 
> Also, does your patch cause the unreachable-code warning to trigger in more
> cases now?
> 
> More comments inline.
> 
>  Index: lib/Analysis/CFG.cpp
> ==========================================================
> =========
> --- lib/Analysis/CFG.cpp (revision 194562)
> +++ lib/Analysis/CFG.cpp (working copy)
> @@ -483,6 +483,192 @@
>      B->addSuccessor(S, cfg->getBumpVectorContext());
>    }
> 
> +  /// \brief Find a relational comparison with an expression evaluating
> + to a  /// boolean and a constant other than 0 and 1.
> +  /// e.g. if ((x < y) == 10)
> +  TryResult checkIncorrectRelationalOperator(const BinaryOperator *B) {
> +    const IntegerLiteral *IntLiteral =
> +        dyn_cast<IntegerLiteral>(B->getLHS()->IgnoreParens());
> +    const Expr *BoolExpr = B->getRHS()->IgnoreParens();
> +    bool IntFirst = true;
> +    if (!IntLiteral) {
> +      IntLiteral = dyn_cast<IntegerLiteral>(B->getRHS()->IgnoreParens());
> +      BoolExpr = B->getLHS()->IgnoreParens();
> +      IntFirst = false;
> +    }
> +
> +    if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue())
> +      return TryResult();
> +
> +    llvm::APInt IntValue = IntLiteral->getValue();
> +    if ((IntValue != 1) && (IntValue != 0)) {
> +      BuildOpts.Observer->compareBoolWithInt(B);
> 
> Check that BuildOpts.Observer is not null before dereferencing.
> 
> +      bool IntLarger = !IntValue.isNegative();
> +      BinaryOperatorKind Bok = B->getOpcode();
> +      if (Bok == BO_GT || Bok == BO_GE) {
> +        return TryResult(IntFirst != IntLarger);
> +      } else {
> +        return TryResult(IntFirst == IntLarger);
> +      }
> +    }
> +    return TryResult();
> +  }
> +
> +  /// Find a equality comparison with an expression evaluating to a
> + boolean and  /// a constant other than 0 and 1.
> +  /// e.g. if (!x == 10)
> +  TryResult checkIncorrectEqualityOperator(const BinaryOperator *B) {
> +    const IntegerLiteral *IntLiteral =
> +        dyn_cast<IntegerLiteral>(B->getLHS()->IgnoreParens());
> +    const Expr *BoolExpr = B->getRHS()->IgnoreParens();
> +
> +    if (!IntLiteral) {
> +      IntLiteral = dyn_cast<IntegerLiteral>(B->getRHS()->IgnoreParens());
> +      BoolExpr = B->getLHS()->IgnoreParens();
> +    }
> +
> +    if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue())
> +      return TryResult();
> +
> +    llvm::APInt IntValue = IntLiteral->getValue();
> +    if ((IntValue != 1) && (IntValue != 0)) {
> +      BuildOpts.Observer->compareBoolWithInt(B);
> +      return TryResult(B->getOpcode() != BO_EQ);
> +    }
> +    return TryResult();
> +  }
> +
> +  bool analyzeLogicOperatorCondition(BinaryOperatorKind Relation,
> +                                     const llvm::APSInt Value1,
> +                                     const llvm::APSInt Value2) {
> 
> A switch statement would be nicer here.  Also, add an assert that the two
> values have the same signedness and bit width.
> 
> +    return (Relation == BO_EQ && (Value1 == Value2)) ||
> +           (Relation == BO_NE && (Value1 != Value2)) ||
> +           (Relation == BO_LT && (Value1 <  Value2)) ||
> +           (Relation == BO_LE && (Value1 <= Value2)) ||
> +           (Relation == BO_GT && (Value1 >  Value2)) ||
> +           (Relation == BO_GE && (Value1 >= Value2));  }
> +
> +  /// \brief Find a pair of comparison expressions with or without
> + parentheses  /// with a shared variable and constants and a logical
> + operator between them  /// that always evaluates to either true or false.
> +  /// e.g. if (x != 3 || x != 4)
> +  TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
> +    const BinaryOperator *LHS =
> +        dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
> +    const BinaryOperator *RHS =
> +        dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
> +    if (!LHS || !RHS)
> +      return TryResult();
> +
> +    if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
> +      return TryResult();
> +
> 
> 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.
> 
> +    BinaryOperatorKind BO1 = LHS->getOpcode();
> +    const DeclRefExpr *Decl1 =
> +        dyn_cast<DeclRefExpr>(LHS->getLHS()->IgnoreParenImpCasts());
> +    const IntegerLiteral *Literal1 =
> +        dyn_cast<IntegerLiteral>(LHS->getRHS()->IgnoreParens());
> +
> +    if (!Decl1 && !Literal1) {
> +      if (BO1 == BO_GT)
> +        BO1 = BO_LT;
> +      else if (BO1 == BO_GE)
> +        BO1 = BO_LE;
> +      else if (BO1 == BO_LT)
> +        BO1 = BO_GT;
> +      else if (BO1 == BO_LE)
> +        BO1 = BO_GE;
> +      Decl1 =
> +          dyn_cast<DeclRefExpr>(LHS->getRHS()->IgnoreParenImpCasts());
> +      Literal1 = dyn_cast<IntegerLiteral>(LHS->getLHS()->IgnoreParens());
> +    }
> +
> +    if (!Decl1 || !Literal1)
> +      return TryResult();
> +
> +    BinaryOperatorKind BO2 = RHS->getOpcode();
> +    const DeclRefExpr *Decl2 =
> +
> + dyn_cast<DeclRefExpr>(RHS->getLHS()->IgnoreImpCasts()-
> >IgnoreParens())
> + ;
> 
> ->IgnoreParenImpCasts() instead of ->IgnoreImpCasts()->IgnoreParens()
> 
> +    const IntegerLiteral *Literal2 =
> +        dyn_cast<IntegerLiteral>(RHS->getRHS()->IgnoreParens());
> +
> +    if (!Decl2 && !Literal2) {
> +      if (BO2 == BO_GT)
> +        BO2 = BO_LT;
> +      else if (BO2 == BO_GE)
> +        BO2 = BO_LE;
> +      else if (BO2 == BO_LT)
> +        BO2 = BO_GT;
> +      else if (BO2 == BO_LE)
> +        BO2 = BO_GE;
> +      Decl2 =
> +          dyn_cast<DeclRefExpr>(RHS->getRHS()->IgnoreImpCasts()-
> >IgnoreParens());
> +      Literal2 = dyn_cast<IntegerLiteral>(RHS->getLHS()->IgnoreParens());
> +    }
> +
> +    if (!Decl2 || !Literal2)
> +      return TryResult();
> +
> +    // Check that it is the same variable on both sides.
> +    if (Decl1->getNameInfo().getAsString() !=
> + Decl2->getNameInfo().getAsString())
> 
> Decl1->getDecl() != Decl2->getDecl()
> 
> +      return TryResult();
> +
> +    // evaluate if expression is always true/false
> +    llvm::APSInt L1,L2;
> +    if (!Literal1->EvaluateAsInt(L1,*Context) ||
> +        !Literal2->EvaluateAsInt(L2,*Context))
> +      return TryResult();
> +
> +    const llvm::APSInt MinValueL1 =
> +        llvm::APSInt::getMinValue(L1.getBitWidth(),L1.isUnsigned());
> +    const llvm::APSInt MaxValueL1 =
> +        llvm::APSInt::getMaxValue(L1.getBitWidth(),L1.isUnsigned());
> +
> +    const llvm::APSInt MinValueL2 =
> +        llvm::APSInt::getMinValue(L2.getBitWidth(),L2.isUnsigned());
> +    const llvm::APSInt MaxValueL2 =
> +        llvm::APSInt::getMaxValue(L2.getBitWidth(),L2.isUnsigned());
> +    bool AlwaysTrue = true, AlwaysFalse = true;
> 
> What is going on here?
> +    for (int Offset = -3; Offset <=3; ++Offset) {
> +      for (int Selvalue = 1; Selvalue <= 2; Selvalue++) {
> +
> +        llvm::APSInt SelInt = (Selvalue==1 ? L1 : L2);
> +        llvm::APSInt SelMin = (Selvalue==1 ? MinValueL1 : MinValueL2);
> +        llvm::APSInt SelMax = (Selvalue==1 ? MaxValueL1 : MaxValueL2);
> +
> +        llvm::APSInt OffsetAPS =
> +            llvm::APSInt(llvm::APInt(SelInt.getBitWidth(), Offset),
> +                         SelInt.isUnsigned());
> +
> +        if ((SelInt + OffsetAPS) > SelMax)
> +          continue;
> +        else if ((SelInt + OffsetAPS) < SelMin)
> +          continue;
> +        else
> +          SelInt += OffsetAPS;
> +
> +        bool Res1,Res2;
> +        Res1 = analyzeLogicOperatorCondition(BO1,SelInt, L1);
> +        Res2 = analyzeLogicOperatorCondition(BO2,SelInt, L2);
> +        if (B->getOpcode() == BO_LAnd) {
> +            AlwaysTrue  &= (Res1 && Res2);
> +            AlwaysFalse &= !(Res1 && Res2);
> +        } else {
> +            AlwaysTrue  &= (Res1 || Res2);
> +            AlwaysFalse &= !(Res1 || Res2);
> +        }
> +      }
> +    }
> +
> +    if(AlwaysTrue || AlwaysFalse) {
> +      BuildOpts.Observer->compareAlwaysTrue(B,AlwaysTrue);
> +      return TryResult(AlwaysTrue);
> +    }
> +
> +    return TryResult();
> +  }
> +
>    /// Try and evaluate an expression to an integer constant.
>    bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) {
>      if (!BuildOpts.PruneTriviallyFalseEdges)
> @@ -565,13 +751,24 @@
>              // is determined by the RHS: X && 0 -> 0, X || 1 -> 1.
>              if (RHS.isTrue() == (Bop->getOpcode() == BO_LOr))
>                return RHS.isTrue();
> +          } else {
> +            TryResult BopRes = checkIncorrectLogicOperator(Bop);
> +            if (BopRes.isKnown())
> +              return BopRes.isTrue();
>            }
>          }
> 
>          return TryResult();
> +      } else if (Bop->isEqualityOp()) {
> +          TryResult BopRes = checkIncorrectEqualityOperator(Bop);
> +          if (BopRes.isKnown())
> +            return BopRes.isTrue();
> +      } else if (Bop->isRelationalOp()) {
> +        TryResult BopRes = checkIncorrectRelationalOperator(Bop);
> +        if (BopRes.isKnown())
> +          return BopRes.isTrue();
>        }
>      }
> -
>      bool Result;
>      if (E->EvaluateAsBooleanCondition(Result, *Context))
>        return Result;
> @@ -1311,6 +1508,8 @@
>      else {
>        RHSBlock->setTerminator(Term);
>        TryResult KnownVal = tryEvaluateBool(RHS);
> +      if (!KnownVal.isKnown())
> +        KnownVal = tryEvaluateBool(B);
>        addSuccessor(RHSBlock, KnownVal.isFalse() ? NULL : TrueBlock);
>        addSuccessor(RHSBlock, KnownVal.isTrue() ? NULL : FalseBlock);
>      }
> Index: lib/Sema/AnalysisBasedWarnings.cpp
> ==========================================================
> =========
> --- lib/Sema/AnalysisBasedWarnings.cpp (revision 194562)
> +++ lib/Sema/AnalysisBasedWarnings.cpp (working copy)
> @@ -77,6 +77,60 @@
>    reachable_code::FindUnreachableCode(AC, UC); }
> 
> +/// \brief Warn on logical operator errors in CFGBuilder class
> +LogicalErrorsHandler : public CFGCallback {
> +  Sema &S;
> +public:
> +  LogicalErrorsHandler(Sema &S) : CFGCallback(),S(S) {}
> +  void compareAlwaysTrue(const BinaryOperator* B, bool isAlwaysTrue) {
> +    if (B->getLHS()->getExprLoc().isMacroID() ||
> +        B->getRHS()->getExprLoc().isMacroID())
> +      return;
> +
> +    const BinaryOperator *LHS =
> +        dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
> +    const BinaryOperator *RHS =
> +        dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
> +
> +    if (LHS)
> +      if (LHS->getLHS()->getExprLoc().isMacroID() ||
> +          LHS->getRHS()->getExprLoc().isMacroID())
> +            return;
> +    if (RHS)
> +      if (RHS->getLHS()->getExprLoc().isMacroID() ||
> +          RHS->getRHS()->getExprLoc().isMacroID())
> +            return;
> +
> +    SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()-
> >getLocEnd());
> +    S.Diag(B->getExprLoc(), diag::warn_operator_always_true_comparison)
> +        << DiagRange << isAlwaysTrue;
> +  }
> +
> +  void compareBoolWithInt(const BinaryOperator* B) {
> +    if (B->getLHS()->getExprLoc().isMacroID() ||
> +        B->getRHS()->getExprLoc().isMacroID())
> +      return;
> +
> +    const BinaryOperator *LHS =
> +        dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
> +    const BinaryOperator *RHS =
> +        dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
> +
> +    if (LHS)
> +      if (LHS->getLHS()->getExprLoc().isMacroID() ||
> +          LHS->getRHS()->getExprLoc().isMacroID())
> +            return;
> +    if (RHS)
> +      if (RHS->getLHS()->getExprLoc().isMacroID() ||
> +          RHS->getRHS()->getExprLoc().isMacroID())
> +            return;
> +
> +    SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()-
> >getLocEnd());
> +    S.Diag(B->getExprLoc(), diag::warn_bool_and_int_comparison) <<
> +DiagRange;
> +  }
> +};
> +
> +
> //===----------------------------------------------------------------------===//
> // Check for missing return value.
> //===----------------------------------------------------------------------===//
> @@ -1723,8 +1777,11 @@
>      bool isTemplateInstantiation = false;
>      if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D))
>        isTemplateInstantiation = Function->isTemplateInstantiation();
> -    if (!isTemplateInstantiation)
> +    if (!isTemplateInstantiation) {
> +      LogicalErrorsHandler Leh(S);
> +      AC.getCFGBuildOptions().Observer = &Leh;
>        CheckUnreachable(S, AC);
> +    }
>    }
> 
>    // Check for thread safety violations
> Index: lib/AST/Expr.cpp
> ==========================================================
> =========
> --- lib/AST/Expr.cpp (revision 194562)
> +++ lib/AST/Expr.cpp (working copy)
> @@ -152,6 +152,8 @@
>      switch (UO->getOpcode()) {
>      case UO_Plus:
>        return UO->getSubExpr()->isKnownToHaveBooleanValue();
> +    case UO_LNot:
> +      return true;
>      default:
>        return false;
>      }
> Index: test/Sema/warn-overlap.c
> ==========================================================
> =========
> --- test/Sema/warn-overlap.c (revision 0)
> +++ test/Sema/warn-overlap.c (working copy)
> 
> Add some tests with different variable types and different constant types.
> 
> @@ -0,0 +1,51 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s
> +
> +#define mydefine 2
> +
> +void f(int x) {
> +  int y = 0;
> +  // > || <
> +  if (x > 2 || x < 1) { }
> +  if (x > 2 || x < 2) { }
> +  if (x != 2 || x != 3) { } // expected-warning {{logical disjunction
> +always evaluates to true}}
> +  if (x > 2 || x < 3) { } // expected-warning {{logical disjunction
> +always evaluates to true}}
> +
> +  if (x > 2 || x <= 1) { }
> +  if (x > 2 || x <= 2) { } // expected-warning {{logical disjunction
> + always evaluates to true}}  if (x > 2 || x <= 3) { } //
> + expected-warning {{logical disjunction always evaluates to true}}
> +
> +  if (x >= 2 || x < 1) { }
> +  if (x >= 2 || x < 2) { } // expected-warning {{logical disjunction
> + always evaluates to true}}  if (x >= 2 || x < 3) { } //
> + expected-warning {{logical disjunction always evaluates to true}}
> +
> +  if (x >= 2 || x <= 1) { } // expected-warning {{logical disjunction
> + always evaluates to true}}  if (x >= 2 || x <= 2) { } //
> + expected-warning {{logical disjunction always evaluates to true}}  if
> + (x >= 2 || x <= 3) { } // expected-warning {{logical disjunction
> + always evaluates to true}}
> +
> +  // > && <
> +  if (x > 2 && x < 1) { }  // expected-warning {{logical disjunction
> + always evaluates to false}}  if (x > 2 && x < 2) { }  //
> + expected-warning {{logical disjunction always evaluates to false}}  if
> + (x > 2 && x < 3) { }  // expected-warning {{logical disjunction always
> + evaluates to false}}
> +
> +  if (x > 2 && x <= 1) { }  // expected-warning {{logical disjunction
> + always evaluates to false}}  if (x > 2 && x <= 2) { }  //
> + expected-warning {{logical disjunction always evaluates to false}}  if
> + (x > 2 && x <= 3) { }
> +
> +  if (x >= 2 && x < 1) { }  // expected-warning {{logical disjunction
> + always evaluates to false}}  if (x >= 2 && x < 2) { }  //
> + expected-warning {{logical disjunction always evaluates to false}}  if
> + (x >= 2 && x < 3) { }
> +
> +  if (x >= 2 && x <= 1) { }  // expected-warning {{logical disjunction
> + always evaluates to false}}  if (x >= 2 && x <= 2) { }  if (x >= 2 &&
> + x <= 3) { }
> +
> +  // !=, ==, ..
> +  if (x != 2 || x != 3) { }  // expected-warning {{logical disjunction always
> evaluates to true}}
> +  if (x != 2 || x < 3) { }   // expected-warning {{logical disjunction always
> evaluates to true}}
> +  if (x == 2 && x == 3) { }  // expected-warning {{logical disjunction always
> evaluates to false}}
> +  if (x == 2 && x > 3) { }   // expected-warning {{logical disjunction always
> evaluates to false}}
> +
> +  if (x == mydefine && x > 3) { }  // no-warning
> +  if (x == 2 && y > 3) { }  // no-warning }
> +
> Index: test/Sema/warn-compint.c
> ==========================================================
> =========
> --- test/Sema/warn-compint.c (revision 0)
> +++ test/Sema/warn-compint.c (working copy)
> @@ -0,0 +1,39 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s
> +
> +#define mydefine 2
> +
> +void err(int x, int y) {
> +
> +    if (!x == 10) {}  // expected-warning {{comparison of a boolean expression
> with an integer other than 0 or 1}}
> +    if (!x != 10) {}  // expected-warning {{comparison of a boolean expression
> with an integer other than 0 or 1}}
> +    if (!x <  10) {}  // expected-warning {{comparison of a boolean expression
> with an integer other than 0 or 1}}
> +    if (!x >  10) {}  // expected-warning {{comparison of a boolean expression
> with an integer other than 0 or 1}}
> +    if (!x >= 10) {}  // expected-warning {{comparison of a boolean expression
> with an integer other than 0 or 1}}
> +    if (!x <= 10) {}  // expected-warning {{comparison of a boolean
> + expression with an integer other than 0 or 1}}
> +
> +    if ((x < y) == 10) {}  // expected-warning {{comparison of a boolean
> expression with an integer other than 0 or 1}}
> +    if ((x < y) != 10) {}  // expected-warning {{comparison of a boolean
> expression with an integer other than 0 or 1}}
> +    if ((x < y) <  10) {}  // expected-warning {{comparison of a boolean
> expression with an integer other than 0 or 1}}
> +    if ((x < y) >  10) {}  // expected-warning {{comparison of a boolean
> expression with an integer other than 0 or 1}}
> +    if ((x < y) >= 10) {}  // expected-warning {{comparison of a boolean
> expression with an integer other than 0 or 1}}
> +    if ((x < y) <= 10) {}  // expected-warning {{comparison of a
> +boolean expression with an integer other than 0 or 1}} }
> +
> +void noerr(int x, int y) {
> +
> +    if (!x == 1) {}
> +    if (!x != 0) {}
> +    if (!x <  1) {}
> +    if (!x >  0) {}
> +    if (!x >= 1) {}
> +    if (!x <= 0) {}
> +
> +    if ((x < y) == 1) {}
> +    if ((x < y) != 0) {}
> +    if ((x < y) <  1) {}
> +    if ((x < y) >  0) {}
> +    if ((x < y) >= 1) {}
> +    if ((x < y) <= 0) {}
> +
> +    if ((x < mydefine) <= 10) {}
> +} // no-warning
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ==========================================================
> =========
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 194562)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -339,6 +339,12 @@
>    InGroup<MissingNoreturn>, DefaultIgnore; def warn_unreachable :
> Warning<"will never be executed">,
>    InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;
> 
> Still don't like the logical disjunction text.  I think it would be confusing for
> users.
> 
> +def warn_operator_always_true_comparison : Warning<
> +  "logical disjunction always evaluates to %select{false|true}0">,
> +  InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore; def
> +warn_bool_and_int_comparison : Warning<
> +  "comparison of a boolean expression with an integer other than 0 or
> +1">,
> +  InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;
> 
> /// Built-in functions.
> def ext_implicit_lib_function_decl : ExtWarn<
> Index: include/clang/Analysis/CFG.h
> ==========================================================
> =========
> --- include/clang/Analysis/CFG.h (revision 194562)
> +++ include/clang/Analysis/CFG.h (working copy)
> @@ -45,6 +45,7 @@
>    class ASTContext;
>    class CXXRecordDecl;
>    class CXXDeleteExpr;
> +  class BinaryOperator;
> 
> /// CFGElement - Represents a top-level expression in a basic block.
> class CFGElement {
> @@ -609,6 +610,16 @@
>    }
> };
> 
> 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?
> 
> +/// \brief CFGCallback defines methods that should be called when a
> +logical /// operator error is found when building the CFG.
> +class CFGCallback {
> +public:
> +  CFGCallback(){}
> +  virtual void compareAlwaysTrue(const BinaryOperator *B, bool
> +isAlwaysTrue) {};
> +  virtual void compareBoolWithInt(const BinaryOperator *B) {};
> +  virtual ~CFGCallback(){}
> +};
> +
> /// CFG - Represents a source-level, intra-procedural CFG that represents
> the ///  control-flow of a Stmt.  The Stmt can represent an entire function
> body, ///  or a single expression.  A CFG will always contain one empty block
> that @@ -627,7 +638,7 @@
>    public:
>      typedef llvm::DenseMap<const Stmt *, const CFGBlock*> ForcedBlkExprs;
>      ForcedBlkExprs **forcedBlkExprs;
> -
> +    CFGCallback* Observer;
>      bool PruneTriviallyFalseEdges;
>      bool AddEHEdges;
>      bool AddInitializers;
> @@ -650,7 +661,7 @@
>      }
> 
>      BuildOptions()
> -    : forcedBlkExprs(0), PruneTriviallyFalseEdges(true)
> +    : forcedBlkExprs(0), Observer(0), PruneTriviallyFalseEdges(true)
>        ,AddEHEdges(false)
>        ,AddInitializers(false)
>        ,AddImplicitDtors(false)
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: incorrectlogic.diff
Type: application/octet-stream
Size: 20470 bytes
Desc: incorrectlogic.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140221/6ce52cba/attachment.obj>


More information about the cfe-commits mailing list