On Fri, Oct 2, 2015 at 6:10 AM, Aaron Ballman via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> On Thu, Oct 1, 2015 at 5:18 PM, Richard Trieu <rtr...@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...@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.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 >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
