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

Wed Aug 12 13:50:35 PDT 2015

On Mon, Aug 10, 2015 at 11:26 AM, 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. :-)
>

Possible that the derived object class could be written in such a way that
it expects some matching invariant between its state and the base class's
(unlikely, admittedly - but for example the derived object's dtor might
walk a collection in the base object and expect to find certain related
things in a derived object's data structures - so if the derived was moved
and the base was not, it could break that invariant in the moved-from
object)

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

Ah, then I'd be more concerned about slicing \/

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

I think I'm starting to tend towards "when making the dtor protected or
virtual, make all the special members protected and defaulted". But yeah,
on the fence - it's sort of like "If a tree falls in the forest" sort of
thing...

> Regardless, it sounds like we want to use
> std::move for the FullDependence move ctor-init, correct?
>

Yeah, I'm OK with that. I'm not adamant about it - I think of this like
writing std::move for a raw pointer member - yeah, we might refactor it to
be a unique_ptr later (granted, that would cause a build break) or
something else, but it doesn't seem important to hedge the code against
such a change. But just habitually using std::move is nice too... *shrug*

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