specialize Extend for Vec with IntoIter

[seanmonstar]

Before, vec.extend(&other_vec) was quite a bit faster than vec.extend(other_vec). This allows extending by consuming a vec to use the same code as extending from a slice.

[alexcrichton]

Could this perhaps elide the unsafety by delegating to the other specialization?

[seanmonstar]

@alexcrichton er, of course. Now delegating to extending from slice.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 75e3607 has been approved by alexcrichton

[frewsxcv]

@bors r-

Travis found an issue with this

[arielb1]

Can't this be a memcpy even when T is not Copy?

[seanmonstar]

@arielb1 I had assumed not, because you'd need to drop items that weren't Copy, but it looks like Vec::append does exactly this.

So, I've refactored to make this make use of the same code as append, and then manually set the iterator at the end. This should mean that items aren't dropped, but the heap space is.

[arielb1]

r? @bluss

[bluss]

Lgtm, memcpy is indeed what we do to move ownership of the elements from the iterator's buffer to the vec's. The request for change is basically a style issue, but I thought it was worthwhile.

[bluss]

We use other here to transfer ownership of the elements, I'd be more comfortable with passing them as other: *mut [T], and it could also be even more clear in the comment that elements are moved from other; especially since elements are not dropped after actual use of the method. (No strong opinion on which *const or *mut to use.)

[seanmonstar]

@bluss updated to fn append_elements(&mut self, other: *const, count: usize).

[alexcrichton]

Looks great! Could you also add some tests which exercise this specific specialization? (and corner cases like empty vectors, zero-sized types, etc)

[seanmonstar]

@alexcrichton there were tests for extend, I've added to them tests for zero-sized types and testing that there is no double drop.

[bluss]

This method does move elements read from other into self's allocation. It should also not talk about dropping, because no T elements need to be dropped or are dropped; they are just moved from being stored in one buffer to the other.

Can the method append_elements use an argument of type *const [T] instead? Coercions from &[T] should make that simple to call in both the calling sites.

[bluss]

(Please change this comment to say that it moves the elements, or at least not say without moving, because it is moving them.)

[seanmonstar]

Updated type to *const [T].

I've changed to docs to the suggested, but the original made more sense to me. It's only half moving the elements. It has directly copied them, but the original buffer has not been modified, so if the caller doesn't do something, they'll have copied potentially uncopiable data, and have data unsafety. 🤷‍♂️

[alexcrichton]

@bors: r+

[bors]

📌 Commit f85a533 has been approved by alexcrichton

[bors]

⌛ Testing commit f85a533 with merge 968ae7b...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 968ae7b to master...