[PATCH] D13157: Teach -Wtautological-overlap-compare about enums

[Aaron Ballman via cfe-commits]

On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu <rtrieu at google.com> wrote:
> rtrieu added a comment.
>
> Next time, add
>
>> Differential Revision: <URL>
>
>
> 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<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;
> +}
> ----------------
> 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<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;
>> >
>> > ```
>> I was under the impression that the `cast<Foo>(Bar)` asserts `isa<Foo>(Bar)` for me, so I thought that asserts like those would just be redundant. Changed to your version anyway :)
> You are correct, 'cast<Foo>(Bar)' does assert 'isa<Foo>(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
>
>
>