<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 18 February 2014 21:51, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><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>
</div></blockquote><div><br></div><div>If we remove the vtable on value there would be 3 or so implementations all of which use switches internally (Value, Instruction and IntrinsicInst, maybe another in Constant somewhere?).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">The deleted special members in IntrinsicInst still look relevant - was there a reason they needed to be removed?<br>
</div></blockquote><div><br></div><div>Instinct to follow the rule of five, since IntrinsicInst now has an implicitly defined default constructor that does the right thing. I tried adding the deleted members back and everything still works correctly, so I assume I should do that.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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>
</div></blockquote><div><br></div><div>I'm not sure. [class.inhctor]/4 "A constructor so declared has the same access as the corresponding constructor in X". If this means that I can't go from protected to public then that's a problem.</div>
<div><br></div><div>We can't use CRTP because we don't want to have multiple bases. I'm hopeful that there's some way to compact this, but keep in mind that we tend to favour obvious code and we're still using C++98.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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>
</div></blockquote><div><br></div><div>They are. Comments added to the places that didn't have them and the destructor was empty.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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"><div><div>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>
</div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><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><font color="#888888">
<div><br></div><div>Nick</div><div><br></div></font></span></div>
<br></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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>
</blockquote></div><br></div></div>