[PATCH] D11911: Initialize base class by move constructor instead of copy constructor

[Aaron Ballman via llvm-commits]

On Mon, Aug 10, 2015 at 1:29 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Aug 10, 2015 at 9:57 AM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> aaron.ballman created this revision.
>> aaron.ballman added a reviewer: dblaikie.
>> aaron.ballman added a subscriber: llvm-commits.
>>
>> The move constructor for FullDependence is initializing the base class via
>> a copy constructor instead of a move constructor. This patch adds a default
>> move constructor to the base class,
>
>
> Defaulted moves still aren't supported by the min-spec of MSVC that LLVM
> requires, I believe? So we have to write them out by hand.

Ah, that's a fair point and easy to rectify.

>>
>> and makes use of it from the derived class instead of
>> half-moving/half-copying the object.
>>
>> I'm not certain if this was accidentally forgotten as part of r243791, or
>> was purposeful.
>
>
> I don't think there was any particular intent on my part. Other than the
> fact that Dependence has only trivial members, so move and copy are the same
> operation, so I don't think this change actually makes any difference to the
> optimization opportunities/behavior/etc?

I agree, this should have no active effect currently. I was more
worried about long-term maintenance. It'd be easy to add something to
Dependence which would be useful for move construction, and then any
FullDependence object is in a half-moved state.

> (we could keep the std::move in the derived class's dtor (& leave the base
> class as-is) - so that if the base class ever grows interesting move
> semantics it'll 'just work' there)

Will it? I think that the default copy constructor in Dependence will
suppress the creation of an implicit move constructor. So just using
std::move() will still call the copy constructor in that case.

~Aaron

>
>>
>>
>> ~Aaron
>>
>> http://reviews.llvm.org/D11911
>>
>> Files:
>>   include/llvm/Analysis/DependenceAnalysis.h
>>
>> Index: include/llvm/Analysis/DependenceAnalysis.h
>> ===================================================================
>> --- include/llvm/Analysis/DependenceAnalysis.h
>> +++ include/llvm/Analysis/DependenceAnalysis.h
>> @@ -71,6 +71,7 @@
>>    class Dependence {
>>    protected:
>>      Dependence(const Dependence &) = default;
>> +    Dependence(Dependence &&) = default;
>>
>>    public:
>>      Dependence(Instruction *Source,
>> @@ -225,7 +226,7 @@
>>                     unsigned Levels);
>>
>>      FullDependence(FullDependence &&RHS)
>> -        : Dependence(RHS), Levels(RHS.Levels),
>> +        : Dependence(std::move(RHS)), Levels(RHS.Levels),
>>            LoopIndependent(RHS.LoopIndependent),
>> Consistent(RHS.Consistent),
>>            DV(std::move(RHS.DV)) {}
>>
>>
>>
>