Author: gbiv Date: Thu Oct 1 13:47:52 2015 New Revision: 249053 URL: http://llvm.org/viewvc/llvm-project?rev=249053&view=rev Log: 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. Modified: cfe/trunk/lib/Analysis/CFG.cpp cfe/trunk/test/Sema/warn-overlap.c Modified: cfe/trunk/lib/Analysis/CFG.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=249053&r1=249052&r2=249053&view=diff ============================================================================== --- cfe/trunk/lib/Analysis/CFG.cpp (original) +++ cfe/trunk/lib/Analysis/CFG.cpp Thu Oct 1 13:47:52 2015 @@ -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<IntegerLiteral>(E)) + return E; + if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts())) + return isa<EnumConstantDecl>(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<const DeclRefExpr *, BinaryOperatorKind, const Expr *> +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<DeclRefExpr>(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<IntegerLiteral>(E1) != isa<IntegerLiteral>(E2)) + return false; + + // Integer literal comparisons, regardless of literal type, are acceptable. + if (isa<IntegerLiteral>(E1)) + return true; + + // IntegerLiterals are handled above and only EnumConstantDecls are expected + // beyond this point + assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2)); + auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl(); + auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl(); + + assert(isa<EnumConstantDecl>(Decl1) && isa<EnumConstantDecl>(Decl2)); + const DeclContext *DC1 = Decl1->getDeclContext(); + const DeclContext *DC2 = Decl2->getDeclContext(); + + assert(isa<EnumDecl>(DC1) && isa<EnumDecl>(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<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()); - } + 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<DeclRefExpr>(RHS->getLHS()->IgnoreParenImpCasts()); - 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()->IgnoreParenImpCasts()); - Literal2 = dyn_cast<IntegerLiteral>(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. Modified: cfe/trunk/test/Sema/warn-overlap.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-overlap.c?rev=249053&r1=249052&r2=249053&view=diff ============================================================================== --- cfe/trunk/test/Sema/warn-overlap.c (original) +++ cfe/trunk/test/Sema/warn-overlap.c Thu Oct 1 13:47:52 2015 @@ -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. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits