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

[David Blaikie via llvm-commits]

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.


> > (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).

Maybe best to just remove the protected defaulted copy ctor instead, since
it doesn't protect anything?


> 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)) {}
> >>
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150810/3d642972/attachment.html>