> 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.ball...@gmail.com> wrote: > On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu <rtr...@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@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits