[PATCH] D13157: Teach -Wtautological-overlap-compare about enums
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
+tryNormalizeBinaryOperator(const BinaryOperator *B) {
+ auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
+E = E->IgnoreParens();
+if (isa(E))
+ return E;
+auto *DR = dyn_cast(E->IgnoreParenImpCasts());
+if (DR == nullptr)
+ return nullptr;
+return isa(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(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(A) != isa(B))
+return false;
+
+ // Integer literal comparisons, regardless of literal type, are acceptable.
+ if (isa(A))
+return true;
+
+ // Currently we're only given EnumConstantDecls or IntegerLiterals
+ auto *C1 = cast(cast(A)->getDecl());
+ auto *C2 = cast(cast(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(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 (BO
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a comment. Thank you for working on this -- I think it's a good cleanup and feature-add! I have a few minor comments, but generally LGTM. You should wait for an okay from Richard, however. Comment at: lib/Analysis/CFG.cpp:54 @@ +53,3 @@ +auto *DR = dyn_cast(E->IgnoreParenImpCasts()); +if (DR == nullptr) + return nullptr; Please don't compare a pointer against nullptr with an equality operator. This can be simplified into: ``` if (const auto *DR = dyn_cast<>) return isa<> ? " return nullptr; ``` Comment at: lib/Analysis/CFG.cpp:97 @@ +96,3 @@ + // Currently we're only given EnumConstantDecls or IntegerLiterals + auto *C1 = cast(cast(A)->getDecl()); + auto *C2 = cast(cast(B)->getDecl()); Are you sure that A and B will only ever be DeclRefExprs? You dyn_cast elsewhere. http://reviews.llvm.org/D13157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
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
+tryNormalizeBinaryOperator(const BinaryOperator *B) {
+ auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
+E = E->IgnoreParens();
+if (isa(E))
+ return E;
+if (auto *DR = dyn_cast(E->IgnoreParenImpCasts()))
+ return isa(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(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(A) != isa(B))
+return false;
+
+ // Integer literal comparisons, regardless of literal type, are acceptable.
+ if (isa(A))
+return true;
+
+ // Currently we're only given EnumConstantDecls or IntegerLiterals
+ auto *C1 = cast(cast(A)->getDecl());
+ auto *C2 = cast(cast(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(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;
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
george.burgess.iv added inline comments. Comment at: lib/Analysis/CFG.cpp:54 @@ +53,3 @@ +auto *DR = dyn_cast(E->IgnoreParenImpCasts()); +if (DR == nullptr) + return nullptr; aaron.ballman wrote: > Please don't compare a pointer against nullptr with an equality operator. > This can be simplified into: > > ``` > if (const auto *DR = dyn_cast<>) > return isa<> ? " > return nullptr; > ``` Thanks for catching that! Comment at: lib/Analysis/CFG.cpp:97 @@ +96,3 @@ + // Currently we're only given EnumConstantDecls or IntegerLiterals + auto *C1 = cast(cast(A)->getDecl()); + auto *C2 = cast(cast(B)->getDecl()); aaron.ballman wrote: > Are you sure that A and B will only ever be DeclRefExprs? You dyn_cast > elsewhere. I'm 100% sure if my code matches my intent exactly. ;) This helper exists solely to make `checkIncorrectLogicOperator` a bit cleaner -- by the time we call it there, we can assume `tryNormalizeBinaryOperator` succeeded on both BinOps, and `tryNormalizeBinaryOperator` only returns `DeclRefExpr`s (that contain `EnumConstantDecl`s) or `IntegerLiteral`s. I see that this is unclear though, so I added a line to the function comment to make this constraint more explicit. If you think it would be better, I can also just manually inline this code in `checkIncorrectLogicOperator`. http://reviews.llvm.org/D13157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
aaron.ballman accepted this revision. aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. This revision is now accepted and ready to land. Thanks, I think this looks good (pending confirmation from Richard). ~Aaron http://reviews.llvm.org/D13157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
rtrieu accepted this revision.
rtrieu added a comment.
Fix the comments, then it should be ready to commit.
Comment at: lib/Analysis/CFG.cpp:49
@@ +48,3 @@
+tryNormalizeBinaryOperator(const BinaryOperator *B) {
+ auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
+E = E->IgnoreParens();
Why is this a lambda instead of a helper function?
Comment at: lib/Analysis/CFG.cpp:88
@@ +87,3 @@
+/// 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
Use E1 and E2 instead of A and B to match the naming of decls below.
Comment at: lib/Analysis/CFG.cpp:98
@@ +97,3 @@
+
+ // Currently we're only given EnumConstantDecls or IntegerLiterals
+ auto *C1 = cast(cast(A)->getDecl());
Change the comment to note that IntegerLiterals are handled above and only
EnumConstantDecls are expected beyond this point.
Comment at: lib/Analysis/CFG.cpp:99-104
@@ +98,8 @@
+ // Currently we're only given EnumConstantDecls or IntegerLiterals
+ auto *C1 = cast(cast(A)->getDecl());
+ auto *C2 = cast(cast(B)->getDecl());
+
+ const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext());
+ const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext());
+ return E1 == E2;
+}
There's a few extra casts in here, plus some blind conversions between types.
Add your assumptions for the types in asserts. Also, DeclContext should use
cast<> to get to Decl types. I recommend the following:
```
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;
```
http://reviews.llvm.org/D13157
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
george.burgess.iv closed this revision.
george.burgess.iv marked 4 inline comments as done.
george.burgess.iv added a comment.
Changed code to address all feedback + committed as r249053. Thanks for the
reviews!
Comment at: lib/Analysis/CFG.cpp:49
@@ +48,3 @@
+tryNormalizeBinaryOperator(const BinaryOperator *B) {
+ auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
+E = E->IgnoreParens();
rtrieu wrote:
> Why is this a lambda instead of a helper function?
Because it's small + super specific to `tryNormalizeBinaryOperator`, and making
it a lambda lets me scope it it only to where it's useful. It's now a helper
function. :)
Comment at: lib/Analysis/CFG.cpp:99-104
@@ +98,8 @@
+ // Currently we're only given EnumConstantDecls or IntegerLiterals
+ auto *C1 = cast(cast(A)->getDecl());
+ auto *C2 = cast(cast(B)->getDecl());
+
+ const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext());
+ const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext());
+ return E1 == E2;
+}
rtrieu wrote:
> There's a few extra casts in here, plus some blind conversions between types.
> Add your assumptions for the types in asserts. Also, DeclContext should use
> cast<> to get to Decl types. I recommend the following:
>
> ```
> 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;
>
> ```
I was under the impression that the `cast(Bar)` asserts `isa(Bar)`
for me, so I thought that asserts like those would just be redundant. Changed
to your version anyway :)
http://reviews.llvm.org/D13157
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
On Thu, Oct 1, 2015 at 2:50 PM, George Burgess IV
wrote:
> george.burgess.iv closed this revision.
> george.burgess.iv marked 4 inline comments as done.
> george.burgess.iv added a comment.
>
> Changed code to address all feedback + committed as r249053. Thanks for the
> reviews!
>
>
>
> Comment at: lib/Analysis/CFG.cpp:49
> @@ +48,3 @@
> +tryNormalizeBinaryOperator(const BinaryOperator *B) {
> + auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
> +E = E->IgnoreParens();
>
> rtrieu wrote:
>> Why is this a lambda instead of a helper function?
> Because it's small + super specific to `tryNormalizeBinaryOperator`, and
> making it a lambda lets me scope it it only to where it's useful. It's now a
> helper function. :)
>
>
> Comment at: lib/Analysis/CFG.cpp:99-104
> @@ +98,8 @@
> + // Currently we're only given EnumConstantDecls or IntegerLiterals
> + auto *C1 = cast(cast(A)->getDecl());
> + auto *C2 = cast(cast(B)->getDecl());
> +
> + const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext());
> + const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext());
> + return E1 == E2;
> +}
>
> rtrieu wrote:
>> There's a few extra casts in here, plus some blind conversions between
>> types. Add your assumptions for the types in asserts. Also, DeclContext
>> should use cast<> to get to Decl types. I recommend the following:
>>
>> ```
>> 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;
>>
>> ```
> I was under the impression that the `cast(Bar)` asserts `isa(Bar)`
> for me, so I thought that asserts like those would just be redundant. Changed
> to your version anyway :)
cast<> already does assert, so those new asserts are redundant and
should be removed. Sorry, I didn't catch that suggestion earlier. I
would recommend:
auto *Decl1 = cast(E1)->getDecl();
auto *Decl2 = cast(E2)->getDecl();
assert(isa(Decl1) && isa(Decl2));
return Decl1->getDeclContext() == Decl2->getDeclContext();
I don't think there's a point to asserting that an EnumConstantDecl
has a declaration context that is an EnumDecl -- if that weren't true,
we'd have broken long before reaching this code.
~Aaron
>
>
> http://reviews.llvm.org/D13157
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
rtrieu added a comment. Next time, add > Differential Revision: to your commit and Phabricator will close the diff automatically. http://llvm.org/docs/Phabricator.html Comment at: lib/Analysis/CFG.cpp:99-104 @@ +98,8 @@ + // Currently we're only given EnumConstantDecls or IntegerLiterals + auto *C1 = cast(cast(A)->getDecl()); + auto *C2 = cast(cast(B)->getDecl()); + + const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext()); + const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext()); + return E1 == E2; +} george.burgess.iv wrote: > rtrieu wrote: > > There's a few extra casts in here, plus some blind conversions between > > types. Add your assumptions for the types in asserts. Also, DeclContext > > should use cast<> to get to Decl types. I recommend the following: > > > > ``` > > 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; > > > > ``` > I was under the impression that the `cast(Bar)` asserts `isa(Bar)` > for me, so I thought that asserts like those would just be redundant. Changed > to your version anyway :) You are correct, 'cast(Bar)' does assert 'isa(Bar)'. However, when Bar is not Foo, using the assert here means the crash will produce a backtrace will point straight to this function instead of an assert that points deep into the casting functions. http://reviews.llvm.org/D13157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu wrote: > rtrieu added a comment. > > Next time, add > >> Differential Revision: > > > to your commit and Phabricator will close the diff automatically. > > http://llvm.org/docs/Phabricator.html > > > > Comment at: lib/Analysis/CFG.cpp:99-104 > @@ +98,8 @@ > + // Currently we're only given EnumConstantDecls or IntegerLiterals > + auto *C1 = cast(cast(A)->getDecl()); > + auto *C2 = cast(cast(B)->getDecl()); > + > + const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext()); > + const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext()); > + return E1 == E2; > +} > > george.burgess.iv wrote: >> rtrieu wrote: >> > There's a few extra casts in here, plus some blind conversions between >> > types. Add your assumptions for the types in asserts. Also, DeclContext >> > should use cast<> to get to Decl types. I recommend the following: >> > >> > ``` >> > 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; >> > >> > ``` >> I was under the impression that the `cast(Bar)` asserts `isa(Bar)` >> for me, so I thought that asserts like those would just be redundant. >> Changed to your version anyway :) > You are correct, 'cast(Bar)' does assert 'isa(Bar)'. However, when > Bar is not Foo, using the assert here means the crash will produce a > backtrace will point straight to this function instead of an assert that > points deep into the casting functions. Doubling the expense for assert builds so that we get a slightly better stack trace in the event our assumptions are wrong doesn't seem like a good tradeoff. It means everyone running an assert build pays the price twice to save a few moments of scanning the backtrace in a situation that's (hopefully) highly unlikely to occur in practice. ~Aaron > > > http://reviews.llvm.org/D13157 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
> Next time, add Differential Revision: to your commit and Phabricator will close the diff automatically. Ooh, shiny. Thanks for letting me know! > Doubling the expense for assert builds so that we get a slightly better stack trace in the event our assumptions are wrong doesn't seem like a good tradeoff I'd think that the optimizer would be able to eliminate the redundant checks for Release+Asserts builds, and that this code wouldn't get hit often enough for it to make a meaningful impact on the execution time of a Debug build of clang. That said, I'm happy to potentially make things a bit faster if that's what we choose to do. :) Richard, do you feel strongly one way or the other? If not, I'll go ahead and take them out. George On Thu, Oct 1, 2015 at 12:03 PM, Aaron Ballman wrote: > On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu wrote: > > rtrieu added a comment. > > > > Next time, add > > > >> Differential Revision: > > > > > > to your commit and Phabricator will close the diff automatically. > > > > http://llvm.org/docs/Phabricator.html > > > > > > > > Comment at: lib/Analysis/CFG.cpp:99-104 > > @@ +98,8 @@ > > + // Currently we're only given EnumConstantDecls or IntegerLiterals > > + auto *C1 = cast(cast(A)->getDecl()); > > + auto *C2 = cast(cast(B)->getDecl()); > > + > > + const TagDecl *E1 = > TagDecl::castFromDeclContext(C1->getDeclContext()); > > + const TagDecl *E2 = > TagDecl::castFromDeclContext(C2->getDeclContext()); > > + return E1 == E2; > > +} > > > > george.burgess.iv wrote: > >> rtrieu wrote: > >> > There's a few extra casts in here, plus some blind conversions > between types. Add your assumptions for the types in asserts. Also, > DeclContext should use cast<> to get to Decl types. I recommend the > following: > >> > > >> > ``` > >> > 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; > >> > > >> > ``` > >> I was under the impression that the `cast(Bar)` asserts > `isa(Bar)` for me, so I thought that asserts like those would just be > redundant. Changed to your version anyway :) > > You are correct, 'cast(Bar)' does assert 'isa(Bar)'. However, > when Bar is not Foo, using the assert here means the crash will produce a > backtrace will point straight to this function instead of an assert that > points deep into the casting functions. > > Doubling the expense for assert builds so that we get a slightly > better stack trace in the event our assumptions are wrong doesn't seem > like a good tradeoff. It means everyone running an assert build pays > the price twice to save a few moments of scanning the backtrace in a > situation that's (hopefully) highly unlikely to occur in practice. > > ~Aaron > > > > > > > http://reviews.llvm.org/D13157 > > > > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
I'm in favor of keeping the asserts around. Several times, I've seen Clang crashers than languish in the bug tracker with an assert in a generic casting function. And no one fixes it until someone pokes at the backtrace to find the source of the bad pointer, at which point it gets fixed quickly. On Thu, Oct 1, 2015 at 1:01 PM, George Burgess IV < george.burgess...@gmail.com> wrote: > > Next time, add Differential Revision: to your commit and > Phabricator will close the diff automatically. > > Ooh, shiny. Thanks for letting me know! > > > Doubling the expense for assert builds so that we get a slightly better > stack trace in the event our assumptions are wrong doesn't seem like a > good tradeoff > > I'd think that the optimizer would be able to eliminate the redundant > checks for Release+Asserts builds, and that this code wouldn't get hit > often enough for it to make a meaningful impact on the execution time of a > Debug build of clang. That said, I'm happy to potentially make things a bit > faster if that's what we choose to do. :) > > Richard, do you feel strongly one way or the other? If not, I'll go ahead > and take them out. > > George > > On Thu, Oct 1, 2015 at 12:03 PM, Aaron Ballman > wrote: > >> On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu wrote: >> > rtrieu added a comment. >> > >> > Next time, add >> > >> >> Differential Revision: >> > >> > >> > to your commit and Phabricator will close the diff automatically. >> > >> > http://llvm.org/docs/Phabricator.html >> > >> > >> > >> > Comment at: lib/Analysis/CFG.cpp:99-104 >> > @@ +98,8 @@ >> > + // Currently we're only given EnumConstantDecls or IntegerLiterals >> > + auto *C1 = cast(cast(A)->getDecl()); >> > + auto *C2 = cast(cast(B)->getDecl()); >> > + >> > + const TagDecl *E1 = >> TagDecl::castFromDeclContext(C1->getDeclContext()); >> > + const TagDecl *E2 = >> TagDecl::castFromDeclContext(C2->getDeclContext()); >> > + return E1 == E2; >> > +} >> > >> > george.burgess.iv wrote: >> >> rtrieu wrote: >> >> > There's a few extra casts in here, plus some blind conversions >> between types. Add your assumptions for the types in asserts. Also, >> DeclContext should use cast<> to get to Decl types. I recommend the >> following: >> >> > >> >> > ``` >> >> > 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; >> >> > >> >> > ``` >> >> I was under the impression that the `cast(Bar)` asserts >> `isa(Bar)` for me, so I thought that asserts like those would just be >> redundant. Changed to your version anyway :) >> > You are correct, 'cast(Bar)' does assert 'isa(Bar)'. >> However, when Bar is not Foo, using the assert here means the crash will >> produce a backtrace will point straight to this function instead of an >> assert that points deep into the casting functions. >> >> Doubling the expense for assert builds so that we get a slightly >> better stack trace in the event our assumptions are wrong doesn't seem >> like a good tradeoff. It means everyone running an assert build pays >> the price twice to save a few moments of scanning the backtrace in a >> situation that's (hopefully) highly unlikely to occur in practice. >> >> ~Aaron >> >> > >> > >> > http://reviews.llvm.org/D13157 >> > >> > >> > >> > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
On Thu, Oct 1, 2015 at 5:18 PM, Richard Trieu wrote: > I'm in favor of keeping the asserts around. Several times, I've seen Clang > crashers than languish in the bug tracker with an assert in a generic > casting function. And no one fixes it until someone pokes at the backtrace > to find the source of the bad pointer, at which point it gets fixed quickly. I think no one fixes it until it becomes a pain point, not because the backtrace is marginally more complex. I still don't think the maintenance burden or performance hit from duplicate logic provides a tangible benefit. That being said, I'm also fine letting it slide -- I just worry about this becoming a pattern in more parts of the code. If that happens, we can have a larger discussion about it then. ~Aaron > > On Thu, Oct 1, 2015 at 1:01 PM, George Burgess IV > wrote: >> >> > Next time, add Differential Revision: to your commit and >> > Phabricator will close the diff automatically. >> >> Ooh, shiny. Thanks for letting me know! >> >> > Doubling the expense for assert builds so that we get a slightly better >> > stack trace in the event our assumptions are wrong doesn't seem like a good >> > tradeoff >> >> I'd think that the optimizer would be able to eliminate the redundant >> checks for Release+Asserts builds, and that this code wouldn't get hit often >> enough for it to make a meaningful impact on the execution time of a Debug >> build of clang. That said, I'm happy to potentially make things a bit faster >> if that's what we choose to do. :) >> >> Richard, do you feel strongly one way or the other? If not, I'll go ahead >> and take them out. >> >> George >> >> On Thu, Oct 1, 2015 at 12:03 PM, Aaron Ballman >> wrote: >>> >>> On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu wrote: >>> > rtrieu added a comment. >>> > >>> > Next time, add >>> > >>> >> Differential Revision: >>> > >>> > >>> > to your commit and Phabricator will close the diff automatically. >>> > >>> > http://llvm.org/docs/Phabricator.html >>> > >>> > >>> > >>> > Comment at: lib/Analysis/CFG.cpp:99-104 >>> > @@ +98,8 @@ >>> > + // Currently we're only given EnumConstantDecls or IntegerLiterals >>> > + auto *C1 = cast(cast(A)->getDecl()); >>> > + auto *C2 = cast(cast(B)->getDecl()); >>> > + >>> > + const TagDecl *E1 = >>> > TagDecl::castFromDeclContext(C1->getDeclContext()); >>> > + const TagDecl *E2 = >>> > TagDecl::castFromDeclContext(C2->getDeclContext()); >>> > + return E1 == E2; >>> > +} >>> > >>> > george.burgess.iv wrote: >>> >> rtrieu wrote: >>> >> > There's a few extra casts in here, plus some blind conversions >>> >> > between types. Add your assumptions for the types in asserts. Also, >>> >> > DeclContext should use cast<> to get to Decl types. I recommend the >>> >> > following: >>> >> > >>> >> > ``` >>> >> > 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; >>> >> > >>> >> > ``` >>> >> I was under the impression that the `cast(Bar)` asserts >>> >> `isa(Bar)` for me, so I thought that asserts like those would just >>> >> be >>> >> redundant. Changed to your version anyway :) >>> > You are correct, 'cast(Bar)' does assert 'isa(Bar)'. >>> > However, when Bar is not Foo, using the assert here means the crash will >>> > produce a backtrace will point straight to this function instead of an >>> > assert that points deep into the casting functions. >>> >>> Doubling the expense for assert builds so that we get a slightly >>> better stack trace in the event our assumptions are wrong doesn't seem >>> like a good tradeoff. It means everyone running an assert build pays >>> the price twice to save a few moments of scanning the backtrace in a >>> situation that's (hopefully) highly unlikely to occur in practice. >>> >>> ~Aaron >>> >>> > >>> > >>> > http://reviews.llvm.org/D13157 >>> > >>> > >>> > >> >> > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13157: Teach -Wtautological-overlap-compare about enums
On Fri, Oct 2, 2015 at 6:10 AM, Aaron Ballman via cfe-commits < cfe-commits@lists.llvm.org> wrote: > On Thu, Oct 1, 2015 at 5:18 PM, Richard Trieu wrote: > > I'm in favor of keeping the asserts around. Several times, I've seen > Clang > > crashers than languish in the bug tracker with an assert in a generic > > casting function. And no one fixes it until someone pokes at the > backtrace > > to find the source of the bad pointer, at which point it gets fixed > quickly. > > I think no one fixes it until it becomes a pain point, not because the > backtrace is marginally more complex. I still don't think the > maintenance burden or performance hit from duplicate logic provides a > tangible benefit. That being said, I'm also fine letting it slide -- I > just worry about this becoming a pattern in more parts of the code. If > that happens, we can have a larger discussion about it then. > I feel about the same here & generally discourage explicit asserts when the intent is already expressed by a cast or similar. > > ~Aaron > > > > > On Thu, Oct 1, 2015 at 1:01 PM, George Burgess IV > > wrote: > >> > >> > Next time, add Differential Revision: to your commit and > >> > Phabricator will close the diff automatically. > >> > >> Ooh, shiny. Thanks for letting me know! > >> > >> > Doubling the expense for assert builds so that we get a slightly > better > >> > stack trace in the event our assumptions are wrong doesn't seem like > a good > >> > tradeoff > >> > >> I'd think that the optimizer would be able to eliminate the redundant > >> checks for Release+Asserts builds, and that this code wouldn't get hit > often > >> enough for it to make a meaningful impact on the execution time of a > Debug > >> build of clang. That said, I'm happy to potentially make things a bit > faster > >> if that's what we choose to do. :) > >> > >> Richard, do you feel strongly one way or the other? If not, I'll go > ahead > >> and take them out. > >> > >> George > >> > >> On Thu, Oct 1, 2015 at 12:03 PM, Aaron Ballman > > >> wrote: > >>> > >>> On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu > wrote: > >>> > rtrieu added a comment. > >>> > > >>> > Next time, add > >>> > > >>> >> Differential Revision: > >>> > > >>> > > >>> > to your commit and Phabricator will close the diff automatically. > >>> > > >>> > http://llvm.org/docs/Phabricator.html > >>> > > >>> > > >>> > > >>> > Comment at: lib/Analysis/CFG.cpp:99-104 > >>> > @@ +98,8 @@ > >>> > + // Currently we're only given EnumConstantDecls or IntegerLiterals > >>> > + auto *C1 = > cast(cast(A)->getDecl()); > >>> > + auto *C2 = > cast(cast(B)->getDecl()); > >>> > + > >>> > + const TagDecl *E1 = > >>> > TagDecl::castFromDeclContext(C1->getDeclContext()); > >>> > + const TagDecl *E2 = > >>> > TagDecl::castFromDeclContext(C2->getDeclContext()); > >>> > + return E1 == E2; > >>> > +} > >>> > > >>> > george.burgess.iv wrote: > >>> >> rtrieu wrote: > >>> >> > There's a few extra casts in here, plus some blind conversions > >>> >> > between types. Add your assumptions for the types in asserts. > Also, > >>> >> > DeclContext should use cast<> to get to Decl types. I recommend > the > >>> >> > following: > >>> >> > > >>> >> > ``` > >>> >> > 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; > >>> >> > > >>> >> > ``` > >>> >> I was under the impression that the `cast(Bar)` asserts > >>> >> `isa(Bar)` for me, so I thought that asserts like those would > just be > >>> >> redundant. Changed to your version anyway :) > >>> > You are correct, 'cast(Bar)' does assert 'isa(Bar)'. > >>> > However, when Bar is not Foo, using the assert here means the crash > will > >>> > produce a backtrace will point straight to this function instead of > an > >>> > assert that points deep into the casting functions. > >>> > >>> Doubling the expense for assert builds so that we get a slightly > >>> better stack trace in the event our assumptions are wrong doesn't seem > >>> like a good tradeoff. It means everyone running an assert build pays > >>> the price twice to save a few moments of scanning the backtrace in a > >>> situation that's (hopefully) highly unlikely to occur in practice. > >>> > >>> ~Aaron > >>> > >>> > > >>> > > >>> > http://reviews.llvm.org/D13157 > >>> > > >>> > > >>> > > >> > >> > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
