<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 10, 2015 at 11:26 AM, 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 Mon, Aug 10, 2015 at 1:57 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Aug 10, 2015 at 10:46 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Aug 10, 2015 at 1:29 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Mon, Aug 10, 2015 at 9:57 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> aaron.ballman created this revision.<br>
>> >> aaron.ballman added a reviewer: dblaikie.<br>
>> >> aaron.ballman added a subscriber: llvm-commits.<br>
>> >><br>
>> >> The move constructor for FullDependence is initializing the base class<br>
>> >> via<br>
>> >> a copy constructor instead of a move constructor. This patch adds a<br>
>> >> default<br>
>> >> move constructor to the base class,<br>
>> ><br>
>> ><br>
>> > Defaulted moves still aren't supported by the min-spec of MSVC that LLVM<br>
>> > requires, I believe? So we have to write them out by hand.<br>
>><br>
>> Ah, that's a fair point and easy to rectify.<br>
>><br>
>> >><br>
>> >> and makes use of it from the derived class instead of<br>
>> >> half-moving/half-copying the object.<br>
>> >><br>
>> >> I'm not certain if this was accidentally forgotten as part of r243791,<br>
>> >> or<br>
>> >> was purposeful.<br>
>> ><br>
>> ><br>
>> > I don't think there was any particular intent on my part. Other than the<br>
>> > fact that Dependence has only trivial members, so move and copy are the<br>
>> > same<br>
>> > operation, so I don't think this change actually makes any difference to<br>
>> > the<br>
>> > optimization opportunities/behavior/etc?<br>
>><br>
>> I agree, this should have no active effect currently. I was more<br>
>> worried about long-term maintenance. It'd be easy to add something to<br>
>> Dependence which would be useful for move construction, and then any<br>
>> FullDependence object is in a half-moved state.<br>
><br>
><br>
> I don't think there's too much risk of problematic zombie states. I can't<br>
> think of too many failure modes where a base class moves instead of copies<br>
> and that presents a problem for the moved-from derived object. There are<br>
> some, but usually you just assume the moved-from state is destroyable and<br>
> not much else, so copy satisfies that condition.<br>
<br>
</div></div>I'd be curious to hear what zombie states present problems (off-list<br>
if you'd like) aside from perf issues. :-)<br></blockquote><div><br></div><div>Possible that the derived object class could be written in such a way that it expects some matching invariant between its state and the base class's (unlikely, admittedly - but for example the derived object's dtor might walk a collection in the base object and expect to find certain related things in a derived object's data structures - so if the derived was moved and the base was not, it could break that invariant in the moved-from object)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>><br>
>> > (we could keep the std::move in the derived class's dtor (& leave the<br>
>> > base<br>
>> > class as-is) - so that if the base class ever grows interesting move<br>
>> > semantics it'll 'just work' there)<br>
>><br>
>> Will it? I think that the default copy constructor in Dependence will<br>
>> suppress the creation of an implicit move constructor.<br>
><br>
><br>
> True - I meant if the author of Dependence decided to add move semantics of<br>
> any kind - but yes, having added the explicit default copy, you don't get<br>
> move for free, which is unfortunate, so adding it in might be nice. I<br>
> wouldn't object & will keep it in mind in other hierarchies where I<br>
> have/will be doing similar things...<br>
><br>
> Sort of a pity, really - the only reason I made the copy ctor protected in<br>
> the base is to avoid an API that's prone to slicing. I suppose that's not<br>
> really an issue so long as the base class is abstract (since the base<br>
> class's copy ctor won't be directly invoked, but only invoked via subclasses<br>
> anyway).<br>
<br>
</span>The base class doesn't appear to be abstract in this case.<br></blockquote><div><br></div><div>Ah, then I'd be more concerned about slicing \/</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Maybe best to just remove the protected defaulted copy ctor instead, since<br>
> it doesn't protect anything?<br>
<br>
</span>Since the object doesn't own any of the pointers it's given, I think<br>
the copy ctor could be removed, though we do lose out on the check for<br>
slicing because of that. I am happy to do whatever you think is the<br>
right answer in this case. </blockquote><div><br></div><div>I think I'm starting to tend towards "when making the dtor protected or virtual, make all the special members protected and defaulted". But yeah, on the fence - it's sort of like "If a tree falls in the forest" sort of thing... <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Regardless, it sounds like we want to use<br>
std::move for the FullDependence move ctor-init, correct?<br></blockquote><div><br>Yeah, I'm OK with that. I'm not adamant about it - I think of this like writing std::move for a raw pointer member - yeah, we might refactor it to be a unique_ptr later (granted, that would cause a build break) or something else, but it doesn't seem important to hedge the code against such a change. But just habitually using std::move is nice too... *shrug*<br><br><br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>> So just using<br>
>> std::move() will still call the copy constructor in that case.<br>
>><br>
>> ~Aaron<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> ~Aaron<br>
>> >><br>
>> >> <a href="http://reviews.llvm.org/D11911" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11911</a><br>
>> >><br>
>> >> Files:<br>
>> >> include/llvm/Analysis/DependenceAnalysis.h<br>
>> >><br>
>> >> Index: include/llvm/Analysis/DependenceAnalysis.h<br>
>> >> ===================================================================<br>
>> >> --- include/llvm/Analysis/DependenceAnalysis.h<br>
>> >> +++ include/llvm/Analysis/DependenceAnalysis.h<br>
>> >> @@ -71,6 +71,7 @@<br>
>> >> class Dependence {<br>
>> >> protected:<br>
>> >> Dependence(const Dependence &) = default;<br>
>> >> + Dependence(Dependence &&) = default;<br>
>> >><br>
>> >> public:<br>
>> >> Dependence(Instruction *Source,<br>
>> >> @@ -225,7 +226,7 @@<br>
>> >> unsigned Levels);<br>
>> >><br>
>> >> FullDependence(FullDependence &&RHS)<br>
>> >> - : Dependence(RHS), Levels(RHS.Levels),<br>
>> >> + : Dependence(std::move(RHS)), Levels(RHS.Levels),<br>
>> >> LoopIndependent(RHS.LoopIndependent),<br>
>> >> Consistent(RHS.Consistent),<br>
>> >> DV(std::move(RHS.DV)) {}<br>
>> >><br>
>> >><br>
>> >><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>