<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 2, 2015 at 6:10 AM, Aaron Ballman via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Oct 1, 2015 at 5:18 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
> I'm in favor of keeping the asserts around. Several times, I've seen Clang<br>
> crashers than languish in the bug tracker with an assert in a generic<br>
> casting function. And no one fixes it until someone pokes at the backtrace<br>
> to find the source of the bad pointer, at which point it gets fixed quickly.<br>
<br>
</span>I think no one fixes it until it becomes a pain point, not because the<br>
backtrace is marginally more complex. I still don't think the<br>
maintenance burden or performance hit from duplicate logic provides a<br>
tangible benefit. That being said, I'm also fine letting it slide -- I<br>
just worry about this becoming a pattern in more parts of the code. If<br>
that happens, we can have a larger discussion about it then.<br></blockquote><div><br></div><div>I feel about the same here & generally discourage explicit asserts when the intent is already expressed by a cast<T> or similar.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
~Aaron<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> On Thu, Oct 1, 2015 at 1:01 PM, George Burgess IV<br>
> <<a href="mailto:george.burgess.iv@gmail.com">george.burgess.iv@gmail.com</a>> wrote:<br>
>><br>
>> > Next time, add Differential Revision: <URL> to your commit and<br>
>> > Phabricator will close the diff automatically.<br>
>><br>
>> Ooh, shiny. Thanks for letting me know!<br>
>><br>
>> > Doubling the expense for assert builds so that we get a slightly better<br>
>> > stack trace in the event our assumptions are wrong doesn't seem like a good<br>
>> > tradeoff<br>
>><br>
>> I'd think that the optimizer would be able to eliminate the redundant<br>
>> checks for Release+Asserts builds, and that this code wouldn't get hit often<br>
>> enough for it to make a meaningful impact on the execution time of a Debug<br>
>> build of clang. That said, I'm happy to potentially make things a bit faster<br>
>> if that's what we choose to do. :)<br>
>><br>
>> Richard, do you feel strongly one way or the other? If not, I'll go ahead<br>
>> and take them out.<br>
>><br>
>> George<br>
>><br>
>> On Thu, Oct 1, 2015 at 12:03 PM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>> On Thu, Oct 1, 2015 at 3:01 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>>> > rtrieu added a comment.<br>
>>> ><br>
>>> > Next time, add<br>
>>> ><br>
>>> >> Differential Revision: <URL><br>
>>> ><br>
>>> ><br>
>>> > to your commit and Phabricator will close the diff automatically.<br>
>>> ><br>
>>> > <a href="http://llvm.org/docs/Phabricator.html" rel="noreferrer" target="_blank">http://llvm.org/docs/Phabricator.html</a><br>
>>> ><br>
>>> ><br>
>>> > ================<br>
>>> > Comment at: lib/Analysis/CFG.cpp:99-104<br>
>>> > @@ +98,8 @@<br>
>>> > + // Currently we're only given EnumConstantDecls or IntegerLiterals<br>
>>> > + auto *C1 = cast<EnumConstantDecl>(cast<DeclRefExpr>(A)->getDecl());<br>
>>> > + auto *C2 = cast<EnumConstantDecl>(cast<DeclRefExpr>(B)->getDecl());<br>
>>> > +<br>
>>> > + const TagDecl *E1 =<br>
>>> > TagDecl::castFromDeclContext(C1->getDeclContext());<br>
>>> > + const TagDecl *E2 =<br>
>>> > TagDecl::castFromDeclContext(C2->getDeclContext());<br>
>>> > + return E1 == E2;<br>
>>> > +}<br>
>>> > ----------------<br>
>>> > george.burgess.iv wrote:<br>
>>> >> rtrieu wrote:<br>
>>> >> > There's a few extra casts in here, plus some blind conversions<br>
>>> >> > between types. Add your assumptions for the types in asserts. Also,<br>
>>> >> > DeclContext should use cast<> to get to Decl types. I recommend the<br>
>>> >> > following:<br>
>>> >> ><br>
>>> >> > ```<br>
>>> >> > assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2));<br>
>>> >> > auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl();<br>
>>> >> > auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl();<br>
>>> >> ><br>
>>> >> > assert(isa<EnumConstantDecl>(Decl1) &&<br>
>>> >> > isa<EnumConstantDecl>(Decl2));<br>
>>> >> > const DeclContext *DC1 = Decl1->getDeclContext();<br>
>>> >> > const DeclContext *DC2 = Decl2->getDeclContext();<br>
>>> >> ><br>
>>> >> > assert(isa<EnumDecl>(DC1) && isa<EnumDecl>(DC2));<br>
>>> >> > return DC1 == DC2;<br>
>>> >> ><br>
>>> >> > ```<br>
>>> >> I was under the impression that the `cast<Foo>(Bar)` asserts<br>
>>> >> `isa<Foo>(Bar)` for me, so I thought that asserts like those would just be<br>
>>> >> redundant. Changed to your version anyway :)<br>
>>> > You are correct, 'cast<Foo>(Bar)' does assert 'isa<Foo>(Bar)'.<br>
>>> > However, when Bar is not Foo, using the assert here means the crash will<br>
>>> > produce a backtrace will point straight to this function instead of an<br>
>>> > assert that points deep into the casting functions.<br>
>>><br>
>>> Doubling the expense for assert builds so that we get a slightly<br>
>>> better stack trace in the event our assumptions are wrong doesn't seem<br>
>>> like a good tradeoff. It means everyone running an assert build pays<br>
>>> the price twice to save a few moments of scanning the backtrace in a<br>
>>> situation that's (hopefully) highly unlikely to occur in practice.<br>
>>><br>
>>> ~Aaron<br>
>>><br>
>>> ><br>
>>> ><br>
>>> > <a href="http://reviews.llvm.org/D13157" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13157</a><br>
>>> ><br>
>>> ><br>
>>> ><br>
>><br>
>><br>
><br>
</div></div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>