<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 10, 2015 at 10:46 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"><span class="">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 via<br>
>> a copy constructor instead of a move constructor. This patch adds a 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>
</span>Ah, that's a fair point and easy to rectify.<br>
<span class=""><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, 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 same<br>
> operation, so I don't think this change actually makes any difference to the<br>
> optimization opportunities/behavior/etc?<br>
<br>
</span>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></blockquote><div><br></div><div>I don't think there's too much risk of problematic zombie states. I can't think of too many failure modes where a base class moves instead of copies and that presents a problem for the moved-from derived object. There are some, but usually you just assume the moved-from state is destroyable and not much else, so copy satisfies that condition.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> (we could keep the std::move in the derived class's dtor (& leave the base<br>
> class as-is) - so that if the base class ever grows interesting move<br>
> semantics it'll 'just work' there)<br>
<br>
</span>Will it? I think that the default copy constructor in Dependence will<br>
suppress the creation of an implicit move constructor. </blockquote><div><br></div><div>True - I meant if the author of Dependence decided to add move semantics of any kind - but yes, having added the explicit default copy, you don't get move for free, which is unfortunate, so adding it in might be nice. I wouldn't object & will keep it in mind in other hierarchies where I have/will be doing similar things...<br><br>Sort of a pity, really - the only reason I made the copy ctor protected in the base is to avoid an API that's prone to slicing. I suppose that's not really an issue so long as the base class is abstract (since the base class's copy ctor won't be directly invoked, but only invoked via subclasses anyway). <br><br>Maybe best to just remove the protected defaulted copy ctor instead, since it doesn't protect anything?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">So just using<br>
std::move() will still call the copy constructor in that case.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><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>
</div></div></blockquote></div><br></div></div>