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

[Aaron Ballman via llvm-commits]

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)) {}
>> >>
>> >>
>> >>
>> >
>
>