<div dir="ltr"><div>> Next time, add Differential Revision: <URL> to your commit and Phabricator will close the diff automatically.</div><div><br></div><div>Ooh, shiny. Thanks for letting me know!</div><div><br></div><div><span style="font-size:12.8000001907349px">> Doubling the expense for assert builds so that we get a slightly </span><span style="font-size:12.8000001907349px">better stack trace in the event our assumptions are wrong doesn't seem </span><span style="font-size:12.8000001907349px">like a good tradeoff</span><br></div><div><span style="font-size:12.8000001907349px"><br></span></div><div><span style="font-size:12.8000001907349px">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. :)</span></div><div><span style="font-size:12.8000001907349px"><br></span></div><div><span style="font-size:12.8000001907349px">Richard, do you feel strongly one way or the other? If not, I'll go ahead and take them out.</span></div><div><span style="font-size:12.8000001907349px"><br></span></div><div><span style="font-size:12.8000001907349px">George</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 1, 2015 at 12:03 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">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 = TagDecl::castFromDeclContext(C1->getDeclContext());<br>
> +  const TagDecl *E2 = 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 between types.  Add your assumptions for the types in asserts.  Also, DeclContext should use cast<> to get to Decl types.  I recommend the 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) && 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 `isa<Foo>(Bar)` for me, so I thought that asserts like those would just be redundant. Changed to your version anyway :)<br>
> 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.<br>
<br>
</div></div>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>
</blockquote></div><br></div>