<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Feb 19, 2014 at 3:46 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"><div class="gmail_extra"><div class="gmail_quote"><div class="">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><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></div></div></blockquote><div><br></div><div>Not sure which things you're referring to in that comment, but the 3 I see initially are IntrinsicInst::Create(..., Instruction*), IntrinsicInst::Create(..., BasicBlock*), and the 3rd isn't /quite/ the same, since it takes a 4th argument, an ArrayRef - so without something more general like perfect forwarding/variadic templates, it wouldn't generalize.<br>
<br>Generalizing the first two with a template might be nice, but since it doesn't generalize over all 3 I can see how that's not so interesting/useful.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="">
<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><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></div></div></blockquote><div><br></div><div>In general everything would "work" because there can't be any callers of these functions in the current codebase - the concern is if callers would appear now that those functions can be called and whether they would be correct. Slicing seems like a reasonable concern and I'd be inclined to delete the special members at the Instruction level, if not all the way up at Value.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="">
<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><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></div></div></blockquote><div><br></div><div>Hmm, fair point (alternatively it's probably OK to keep them protected and just have the derived classes friend IntrinsicInst since the ::Create functions are the only place that's meant to be building them I assume?)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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.<br>
</div></div></div></div></blockquote><div><br></div><div>I wasn't suggesting having multiple bases - something like this:<br><br>class IntrinsicInst : ... {<br> ...<br>};<br><br><br>// template<tyepname T> // and when I got to here I realized what you meant about multiple bases because you have not just one level of derivation, but things like MemIntrinsic that are intermediate<br>
<br>// OK, so how about this:<br><br>template<typename Base, typename Derived><br>class ValueCloneHelper : public Base {<br>protected:<br> Derived *clone_impl() const LLVM_OVERRIDE {</div><div> return new Derived(static_cast<const Derived&>(*this));</div>
<div> }<br>};<br><br>class MemIntrinsic : public ValueCloneHelper<IntrinsicInst, MemIntrinsic> {</div><div>};<br><br>class MemSetInst : public ValueCloneHelper<MemIntrinsic, MemSetInst> {</div><div>};<br><br>
, etc... <br><br>Now, granted, those derived types either have to publicly copy constructible, which could risk other clients slicing, or they'd have to friend the ValueCloneHelper base.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div class="">
<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><div>They are. Comments added to the places that didn't have them and the destructor was empty.</div></div></div></div></blockquote><div><br>Sounds good.<br><br>- Dave</div></div>
</div></div>