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

[George Burgess IV via cfe-commits]

> Next time, add Differential Revision: <URL> 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 <aaron.ballman at gmail.com>
wrote:

> 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
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151001/a4b40eb1/attachment.html>