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

[Aaron Ballman via llvm-commits]

Pinging the thread based on a request from dblaikie. :-)

~Aaron

On Mon, Aug 10, 2015 at 2:26 PM, Aaron Ballman <aaron.ballman at gmail.com> wrote:
> On Mon, Aug 10, 2015 at 1:57 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>> On Mon, Aug 10, 2015 at 10:46 AM, Aaron Ballman <aaron.ballman at gmail.com>
>> wrote:
>>>
>>> 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.
>>
>>
>> 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.
>
> I'd be curious to hear what zombie states present problems (off-list
> if you'd like) aside from perf issues. :-)
>
>>>
>>> > (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.
>>
>>
>> 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...
>>
>> 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).
>
> The base class doesn't appear to be abstract in this case.
>
>> Maybe best to just remove the protected defaulted copy ctor instead, since
>> it doesn't protect anything?
>
> Since the object doesn't own any of the pointers it's given, I think
> the copy ctor could be removed, though we do lose out on the check for
> slicing because of that. I am happy to do whatever you think is the
> right answer in this case. Regardless, it sounds like we want to use
> std::move for the FullDependence move ctor-init, correct?
>
> ~Aaron
>
>>
>>>
>>> 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)) {}
>>> >>
>>> >>
>>> >>
>>> >
>>
>>