[PATCH][ADT] Add move-construction and move-assignment from SmallVectorImpl<T> to SmallVector<T, N>.
Lang Hames
lhames at gmail.com
Thu Jan 22 20:05:32 PST 2015
Hi Dave,
> an O(N) move for SmallVector would be a performance bug.
I'm inclined to argue that that's distinct from a correctness bug (in the sense I was using the term), but either way it seems reasonable to test it here. ;)
> several of the tests in this file call "reset()" after "makeSequence"
I missed that when I was flicking through the tests - I withdraw my objection. Still, I'm going to commit this with my suggested approach. If you think the other approach seems more reasonable feel free to switch it.
> (I noticed the copy operations went missing from the original patch - were they already committed? Did you want to commit them? They seem reasonable/handy/etc - just curious)
They don't have any users (or even potential users) yet, so I left them out to keep this change minimal. I have no objection to them going in though. If you think they're warranted I'll add them in a follow-up patch (if you don't beat me to it).
Cheers,
Lang.
> On Jan 22, 2015, at 6:52 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
>> On Thu, Jan 22, 2015 at 6:24 PM, Lang Hames <lhames at gmail.com> wrote:
>> Hi Guys,
>>
>> I was attempting to pre-empt this with "I'm still just testing correctness, rather than implementation specifics". ;)
>
> For std::vector at least, this is an important guarantee because it means std::vector's move operations don't require move/copy operations from its elements (you can have an immovable object in a std::vector and still move the vector around). That's not the case for SmallVector owing to the small-portion, but I think this is more than implementation specifics - an O(N) move for SmallVector would be a performance bug.
>
>> 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);
>
> Fair enough - makes the test generic over the different big/small pairs. I kind of like the "no ctors were called in the making of this move" idea, but that's just me.
>
>> 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.
>
> Can't say I've looked - I thought a few tests either reset or saved counter state between each step to test that certain operations had certain effects (or not). A quick skim seems to indicate this is actually common - several of the tests in this file call "reset()" after "makeSequence" which seems expected/reasonable.
>
>> How does that sound?
>
> All good.
>
> (I noticed the copy operations went missing from the original patch - were they already committed? Did you want to commit them? They seem reasonable/handy/etc - just curious)
>
>>
>> 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/2bcafe82/attachment.html>
More information about the llvm-commits
mailing list