[PATCH][ADT] Add move-construction and move-assignment from SmallVectorImpl<T> to SmallVector<T,N>.

[David Blaikie]

On Thu, Jan 22, 2015 at 5:14 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> > On 2015-Jan-22, at 17:11, Duncan P. N. Exon Smith <dexonsmith at apple.com>
> wrote:
> >
> > This tests that `Constructable` was moved, but it still doesn't test
> > whether the heap allocation was moved (as opposed to individual
> > elements being moved to a new allocation).  You need something like
> > this to test that:
> >
> >     SmallVector<int, 4> V1(8), V2;
> >     auto First = V1.begin();
> >     V2 = std::move(V1);
> >     EXPECT_EQ(First, V2.begin());
>
> Or maybe you could check that no constructors had been called at all
> in the move?
>

This. That's the guarantee I expect from moving a container with dynamic
storage - no operations on my elements should be performed at all.


>
> >
> > Of course, you only want to check this when you expect the whole
> > allocation to move over: when the source vector is in big mode.
> >
> >> On 2015-Jan-22, at 16:43, Lang Hames <lhames at gmail.com> wrote:
> >>
> >> Hi Guys,
> >>
> >> Thanks for catching that - I've attached an updated patch. I'm still
> just testing correctness, rather than implementation specifics, but it does
> at least verify that it's move-constructing rather than copy-constructing
> now.
> >>
> >> Cheers,
> >> Lang.
> >>
> >>
> >> On Thu, Jan 22, 2015 at 4:19 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >>
> >> On Thu, Jan 22, 2015 at 4:17 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >>
> >>> On 2015-Jan-22, at 16:04, David Blaikie <dblaikie at gmail.com> wrote:
> >>>
> >>> This looks like it's only adding copy, not move - did I miss something?
> >>>
> >>> On Thu, Jan 22, 2015 at 3:54 PM, Lang Hames <lhames at gmail.com> wrote:
> >>> Hi All,
> >>>
> >>> The attached patch adds move-construction and move-assignment from
> SmallVectorImpl<T> to SmallVector<T, N>. It enables us to move between
> SmallVectors of different types, which will allow me to fix one of the most
> commented-upon warts from the new JIT APIs.
> >>>
> >>> Unit-test included.
> >>>
> >>> Cheers,
> >>> Lang.
> >>>
> >>
> >> Yeah, the tests don't actually confirm that the thing was moved.  In
> >> particular, you want to check that whenever the source is in big-mode,
> >> the destination points at the same memory that the source used to (well,
> >> assuming that's the behaviour you want, which the naive implementation
> >> will give you IIUC).
> >>
> >> There's also a "big mode -> small mode" funny state thing that'll happen
> >> with the naive implementation.  I'm not sure it's *wrong*, but...
> >>
> >> Given:
> >>
> >>    SmallVector<int, 1> InBigMode(2);
> >>    SmallVector<int, 2> InBigModeToo = std::move(InBigMode);
> >>
> >> I think this will pass:
> >>
> >>    assert(!InBigModeToo.isSmall());
> >>
> >> This a new state -- a SmallVector can be in "big" mode with a heap
> >> allocation, even though its capacity would fit in its "small" mode
> >> stack allocation.
> >>
> >> While weird, I don't really have a problem with it (but maybe others
> >> do?).  Might be worth testing a few operations in this untested state
> >> though.
> >>
> >> It's probably valuable that this is the outcome - it'd be wasteful to
> throw away the dynamic allocation (going down to small mode) - better to
> keep it even though it's < small_size. So I'd be in favor of this.
> >>
> >>
> >> <SmallVectorMoveAssignmentFromSmallVectorImpl-2.patch>
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150122/7a3bcf9f/attachment.html>