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

David Blaikie via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 2 14:13:23 PDT 2015


On Fri, Oct 2, 2015 at 6:10 AM, Aaron Ballman via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> 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.
>

I feel about the same here & generally discourage explicit asserts when the
intent is already expressed by a cast<T> or similar.


>
> ~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
> >>> >
> >>> >
> >>> >
> >>
> >>
> >
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151002/1922ca78/attachment.html>


More information about the cfe-commits mailing list