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

[Lang Hames]

Hi Guys,

I was attempting to pre-empt this with  "I'm still just testing correctness, rather than implementation specifics". ;)

If we want to verify the implementation behavior for head-allocated source vectors in this test, then my inclination would be to use something like:

Constructable *OrigData = Source.data();
// Do move...
EXPECT_TRUE(Source.isSmall() || Dest.data() == OrigData);

If that is true it implies that nothing was moved, but it doesn't require a counter-reset in the middle of the function to zero out the construction count, which would cause me to do a double take if I came across it.

How does that sound?

Cheers,
Lang.

> On Jan 22, 2015, at 5:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
>> 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/6a010c84/attachment.html>