From: George Burgess IV Date: Thu, 1 Oct 2015 18:47:52 +0000 (+0000) Subject: Teach -Wtautological-overlap-compare about enums X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a1eeb192a5d15a5fb60f9ed90a1b45e94625ca39;p=clang Teach -Wtautological-overlap-compare about enums Prior to this patch, -Wtautological-overlap-compare would only warn us if there was a sketchy logical comparison between variables and IntegerLiterals. This patch makes -Wtautological-overlap-compare aware of EnumConstantDecls, so it can apply the same logic to them. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@249053 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Analysis/CFG.cpp b/lib/Analysis/CFG.cpp index 2a988c6262..6cb63f2b17 100644 --- a/lib/Analysis/CFG.cpp +++ b/lib/Analysis/CFG.cpp @@ -39,6 +39,78 @@ static SourceLocation GetEndLoc(Decl *D) { return D->getLocation(); } +/// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral +/// or EnumConstantDecl from the given Expr. If it fails, returns nullptr. +const Expr *tryTransformToIntOrEnumConstant(const Expr *E) { + E = E->IgnoreParens(); + if (isa(E)) + return E; + if (auto *DR = dyn_cast(E->IgnoreParenImpCasts())) + return isa(DR->getDecl()) ? DR : nullptr; + return nullptr; +} + +/// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is +/// an integer literal or an enum constant. +/// +/// If this fails, at least one of the returned DeclRefExpr or Expr will be +/// null. +static std::tuple +tryNormalizeBinaryOperator(const BinaryOperator *B) { + BinaryOperatorKind Op = B->getOpcode(); + + const Expr *MaybeDecl = B->getLHS(); + const Expr *Constant = tryTransformToIntOrEnumConstant(B->getRHS()); + // Expr looked like `0 == Foo` instead of `Foo == 0` + if (Constant == nullptr) { + // Flip the operator + if (Op == BO_GT) + Op = BO_LT; + else if (Op == BO_GE) + Op = BO_LE; + else if (Op == BO_LT) + Op = BO_GT; + else if (Op == BO_LE) + Op = BO_GE; + + MaybeDecl = B->getRHS(); + Constant = tryTransformToIntOrEnumConstant(B->getLHS()); + } + + auto *D = dyn_cast(MaybeDecl->IgnoreParenImpCasts()); + return std::make_tuple(D, Op, Constant); +} + +/// For an expression `x == Foo && x == Bar`, this determines whether the +/// `Foo` and `Bar` are either of the same enumeration type, or both integer +/// literals. +/// +/// It's an error to pass this arguments that are not either IntegerLiterals +/// or DeclRefExprs (that have decls of type EnumConstantDecl) +static bool areExprTypesCompatible(const Expr *E1, const Expr *E2) { + // User intent isn't clear if they're mixing int literals with enum + // constants. + if (isa(E1) != isa(E2)) + return false; + + // Integer literal comparisons, regardless of literal type, are acceptable. + if (isa(E1)) + return true; + + // IntegerLiterals are handled above and only EnumConstantDecls are expected + // beyond this point + assert(isa(E1) && isa(E2)); + auto *Decl1 = cast(E1)->getDecl(); + auto *Decl2 = cast(E2)->getDecl(); + + assert(isa(Decl1) && isa(Decl2)); + const DeclContext *DC1 = Decl1->getDeclContext(); + const DeclContext *DC2 = Decl2->getDeclContext(); + + assert(isa(DC1) && isa(DC2)); + return DC1 == DC2; +} + class CFGBuilder; /// The CFG builder uses a recursive algorithm to build the CFG. When @@ -694,56 +766,35 @@ private: if (!LHS->isComparisonOp() || !RHS->isComparisonOp()) return TryResult(); - BinaryOperatorKind BO1 = LHS->getOpcode(); - const DeclRefExpr *Decl1 = - dyn_cast(LHS->getLHS()->IgnoreParenImpCasts()); - const IntegerLiteral *Literal1 = - dyn_cast(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(LHS->getRHS()->IgnoreParenImpCasts()); - Literal1 = dyn_cast(LHS->getLHS()->IgnoreParens()); - } + const DeclRefExpr *Decl1; + const Expr *Expr1; + BinaryOperatorKind BO1; + std::tie(Decl1, BO1, Expr1) = tryNormalizeBinaryOperator(LHS); - if (!Decl1 || !Literal1) + if (!Decl1 || !Expr1) return TryResult(); - BinaryOperatorKind BO2 = RHS->getOpcode(); - const DeclRefExpr *Decl2 = - dyn_cast(RHS->getLHS()->IgnoreParenImpCasts()); - const IntegerLiteral *Literal2 = - dyn_cast(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(RHS->getRHS()->IgnoreParenImpCasts()); - Literal2 = dyn_cast(RHS->getLHS()->IgnoreParens()); - } + const DeclRefExpr *Decl2; + const Expr *Expr2; + BinaryOperatorKind BO2; + std::tie(Decl2, BO2, Expr2) = tryNormalizeBinaryOperator(RHS); - if (!Decl2 || !Literal2) + if (!Decl2 || !Expr2) return TryResult(); // Check that it is the same variable on both sides. if (Decl1->getDecl() != Decl2->getDecl()) return TryResult(); + // Make sure the user's intent is clear (e.g. they're comparing against two + // int literals, or two things from the same enum) + if (!areExprTypesCompatible(Expr1, Expr2)) + return TryResult(); + llvm::APSInt L1, L2; - if (!Literal1->EvaluateAsInt(L1, *Context) || - !Literal2->EvaluateAsInt(L2, *Context)) + if (!Expr1->EvaluateAsInt(L1, *Context) || + !Expr2->EvaluateAsInt(L2, *Context)) return TryResult(); // Can't compare signed with unsigned or with different bit width. diff --git a/test/Sema/warn-overlap.c b/test/Sema/warn-overlap.c index 44d6ad5efa..1e8a614d0f 100644 --- a/test/Sema/warn-overlap.c +++ b/test/Sema/warn-overlap.c @@ -2,6 +2,16 @@ #define mydefine 2 +enum Choices { + CHOICE_0 = 0, + CHOICE_1 = 1 +}; + +enum Unchoices { + UNCHOICE_0 = 0, + UNCHOICE_1 = 1 +}; + void f(int x) { int y = 0; @@ -54,6 +64,30 @@ void f(int x) { if (x == mydefine && x > 3) { } if (x == (mydefine + 1) && x > 3) { } + + if (x != CHOICE_0 || x != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + + // Don't warn if comparing x to different types + if (x == CHOICE_0 && x == 1) { } + if (x != CHOICE_0 || x != 1) { } + + // "Different types" includes different enums + if (x == CHOICE_0 && x == UNCHOICE_1) { } + if (x != CHOICE_0 || x != UNCHOICE_1) { } +} + +void enums(enum Choices c) { + if (c != CHOICE_0 || c != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + + // Don't warn if comparing x to different types + if (c == CHOICE_0 && c == 1) { } + if (c != CHOICE_0 || c != 1) { } + + // "Different types" includes different enums + if (c == CHOICE_0 && c == UNCHOICE_1) { } + if (c != CHOICE_0 || c != UNCHOICE_1) { } } // Don't generate a warning here.