From ced56e6eca30fc90f82be37569d8e6fa505bd9d4 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Thu, 1 Oct 2015 18:47:52 +0000 Subject: 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. llvm-svn: 249053 --- clang/lib/Analysis/CFG.cpp | 127 +++++++++++++++++++++++++++++++-------------- 1 file changed, 89 insertions(+), 38 deletions(-) (limited to 'clang/lib/Analysis/CFG.cpp') diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 2a988c6262e..6cb63f2b175 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/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. -- cgit v1.2.3