r249053 - Teach -Wtautological-overlap-compare about enums
George Burgess IV via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 1 11:47:53 PDT 2015
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.
More information about the cfe-commits
mailing list