<div dir="ltr">Hi Guys,<div><br></div><div>Thanks for the patch review. I've committed the move operations in r226899.</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 22, 2015 at 8:05 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div>Hi Dave,</div><div><br></div><div><span class=""><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><font color="#000000"><span style="background-color:rgba(255,255,255,0)">an O(N) move for SmallVector would be a performance bug.</span></font></div></div></div></blockquote><div><br></div></span>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. ;)<span class=""><br><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><font color="#000000"><span style="background-color:rgba(255,255,255,0)">several of the tests in this file call "reset()" after "makeSequence" </span></font></div></div></div></blockquote><div><br></div></span>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.</div><span class=""><div><br></div><div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><font color="#000000"><span style="background-color:rgba(255,255,255,0)">(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)</span></font></div></div></div></blockquote><br></div></span><div>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).</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><div class="h5"><div><br></div><div>On Jan 22, 2015, at 6:52 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><br></div><blockquote type="cite"><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 22, 2015 at 6:24 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div>Hi Guys,</div><div><br></div><div>I was attempting to pre-empt this with "<span style="background-color:rgba(255,255,255,0)">I'm still just testing correctness, rather than implementation specifics". ;)</span></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div>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:<br></div><div><br></div><div>Constructable *OrigData = Source.data();</div><div>// Do move...</div><div>EXPECT_TRUE(Source.isSmall() || Dest.data() == OrigData);</div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div>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.<br></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div></div><div>How does that sound?<br></div></div></blockquote><div><br>All good.<br><br>(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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div></div><div><br></div><div>Cheers,</div><div>Lang.</div><div><div><div><br></div><div>On Jan 22, 2015, at 5:41 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><br></div><blockquote type="cite"><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 22, 2015 at 5:14 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>> On 2015-Jan-22, at 17:11, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
> This tests that `Constructable` was moved, but it still doesn't test<br>
> whether the heap allocation was moved (as opposed to individual<br>
> elements being moved to a new allocation). You need something like<br>
> this to test that:<br>
><br>
> SmallVector<int, 4> V1(8), V2;<br>
> auto First = V1.begin();<br>
> V2 = std::move(V1);<br>
> EXPECT_EQ(First, V2.begin());<br>
<br>
</span>Or maybe you could check that no constructors had been called at all<br>
in the move?<br></blockquote><div><br>This. That's the guarantee I expect from moving a container with dynamic storage - no operations on my elements should be performed at all.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
><br>
> Of course, you only want to check this when you expect the whole<br>
> allocation to move over: when the source vector is in big mode.<br>
><br>
>> On 2015-Jan-22, at 16:43, Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
>><br>
>> Hi Guys,<br>
>><br>
>> 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.<br>
>><br>
>> Cheers,<br>
>> Lang.<br>
>><br>
>><br>
>> On Thu, Jan 22, 2015 at 4:19 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>> On Thu, Jan 22, 2015 at 4:17 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>>> On 2015-Jan-22, at 16:04, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>> This looks like it's only adding copy, not move - did I miss something?<br>
>>><br>
>>> On Thu, Jan 22, 2015 at 3:54 PM, Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
>>> Hi All,<br>
>>><br>
>>> 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.<br>
>>><br>
>>> Unit-test included.<br>
>>><br>
>>> Cheers,<br>
>>> Lang.<br>
>>><br>
>><br>
>> Yeah, the tests don't actually confirm that the thing was moved. In<br>
>> particular, you want to check that whenever the source is in big-mode,<br>
>> the destination points at the same memory that the source used to (well,<br>
>> assuming that's the behaviour you want, which the naive implementation<br>
>> will give you IIUC).<br>
>><br>
>> There's also a "big mode -> small mode" funny state thing that'll happen<br>
>> with the naive implementation. I'm not sure it's *wrong*, but...<br>
>><br>
>> Given:<br>
>><br>
>> SmallVector<int, 1> InBigMode(2);<br>
>> SmallVector<int, 2> InBigModeToo = std::move(InBigMode);<br>
>><br>
>> I think this will pass:<br>
>><br>
>> assert(!InBigModeToo.isSmall());<br>
>><br>
>> This a new state -- a SmallVector can be in "big" mode with a heap<br>
>> allocation, even though its capacity would fit in its "small" mode<br>
>> stack allocation.<br>
>><br>
>> While weird, I don't really have a problem with it (but maybe others<br>
>> do?). Might be worth testing a few operations in this untested state<br>
>> though.<br>
>><br>
>> 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.<br>
>><br>
>><br>
>> <SmallVectorMoveAssignmentFromSmallVectorImpl-2.patch><br>
><br>
><br>
</div></div><div><div>> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>
</div></blockquote></div></div></div></blockquote></div><br></div></div>
</div></blockquote></div></div></div></blockquote></div><br></div>