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

[Richard Trieu via cfe-commits]

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.iv at gmail.com> wrote:

> > 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/1e98ec0f/attachment.html>