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

[Aaron Ballman via cfe-commits]

On Thu, Oct 1, 2015 at 5:18 PM, Richard Trieu <rtrieu at google.com> 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
> <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
>>> >
>>> >
>>> >
>>
>>
>