george.burgess.iv updated this revision to Diff 35744.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Addressed all feedback
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,71 @@
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;
+ if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
+ return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
+ return 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.
+///
+/// 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 *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 +759,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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
