george.burgess.iv created this revision.
george.burgess.iv added a reviewer: rtrieu.
george.burgess.iv added a subscriber: cfe-commits.
Currently, -Wtautological-overlap-compare only emits warnings if the
comparisons are between integer literals and variables. This patch adds support
for comparison between variables and enums if the user's intent seems
moderately obvious.
Richard -- I chose you for review because it looks like you touched the code
last. If you're busy, I'm happy to petition others :)
http://reviews.llvm.org/D13157
Files:
lib/Analysis/CFG.cpp
test/Sema/warn-overlap.c
Index: test/Sema/warn-overlap.c
===================================================================
--- test/Sema/warn-overlap.c
+++ 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 @@
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.
Index: lib/Analysis/CFG.cpp
===================================================================
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -39,6 +39,69 @@
return D->getLocation();
}
+/// 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) {
+ auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
+ E = E->IgnoreParens();
+ if (isa<IntegerLiteral>(E))
+ return E;
+ auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts());
+ if (DR == nullptr)
+ return nullptr;
+ return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
+ };
+
+ 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.
+static bool areExprTypesCompatible(const Expr *A, const Expr *B) {
+ // User intent isn't clear if they're mixing int literals with enum
+ // constants.
+ if (isa<IntegerLiteral>(A) != isa<IntegerLiteral>(B))
+ return false;
+
+ // Integer literal comparisons, regardless of literal type, are acceptable.
+ if (isa<IntegerLiteral>(A))
+ return true;
+
+ // Currently we're only given EnumConstantDecls or IntegerLiterals
+ auto *C1 = cast<EnumConstantDecl>(cast<DeclRefExpr>(A)->getDecl());
+ auto *C2 = cast<EnumConstantDecl>(cast<DeclRefExpr>(B)->getDecl());
+
+ const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext());
+ const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext());
+ return E1 == E2;
+}
+
class CFGBuilder;
/// The CFG builder uses a recursive algorithm to build the CFG. When
@@ -694,56 +757,35 @@
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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
