<div dir="ltr">The 3 create functions look like they could be refactored into a template, perhaps? (even just as an implementation detail inside the .cpp file) even clone_impl could use such a thing - but clone_impl might be better implemented via a small CRTP intermediate template class (that way you don't have to do virtual dispatch as well as switching)?<br>
<br>The deleted special members in IntrinsicInst still look relevant - was there a reason they needed to be removed?<br><br>Having to repeat the same set of ctors in every CallInst is a bit boring - could just template the 4th parameter and forward up to the base? Eventually these could/should be replaced with inheriting ctors, I assume?<br>
<br>I assume the virtua dtors are to create anchors/key functions? Maybe a comment explaining that (or maybe not? it is pretty common in the codebase)<br><br>Thanks for looking into this - when I asked about it on IRC I wasn't really trying to cause work to happen, just explain some loose ends in the debug info. Though this'll only account for13 of my 91 missing types... I might hunt for the rest one day.</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Feb 18, 2014 at 9:27 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">LLVM's hand-rolled RTTI differs from C++'s in that it allows you to call the construction method on a base class and then based on the dynamic arguments decide to construct a more derived type and return a pointer to that. This looks like calling CallInst::Create and getting back something you can cast<MemSetInst>(). However, we implement this by new'ing a CallInst, which breaks C++ aliasing rules.<div>


<br></div><div>The attached patch fixes this undefined behaviour by making CallInst forward to IntrinsicInst to construct intrinsics, which in turn uses private constructors on the most derived types. There is some runtime cost, as the implementation of CallInst::Create moved from the header to the .cpp file. If this is a serious problem, let me know and I can probably fix that, I just think the result will result in less much readable code.</div>


<div><br></div><div>Please review!</div><div><br></div><div>As a related issue, I notice that Value has an unnecessary vtable. There's no pressing need to get rid of either form of RTTI (nobody uses dynamic_cast, just normal virtual dispatch), but I want to understand how this happened and whether it's okay to use going forward, or if that happened while nobody was looking and I should go hammer it out of there. It looks like removing it will be a mechanical change.</div>
<span class="HOEnZb"><font color="#888888">

<div><br></div><div>Nick</div><div><br></div></font></span></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>