add append to vec with tests and benchmarks

[JeffBelgum]

Please review carefully. Contains unsafe and is my first commit to Rust.

Uses ptr::copy_nonoverlapping_memory. Attempts to handle zero-size types correctly.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[Gankra]

Does vec![] not infer correctly here?

[Gankra]

Generally we only add benchmarks when we're trying to establish that a change has a desired performance impact. Adding them for every feature is kinda unwieldy (just slows everything else down).

[Gankra]

Actual code seems legit, just style nits.

👍

[JeffBelgum]

I've addressed the previous comments.

[Gankra]

Oh derp. Yeah no worries.

[Gankra]

I would still drop these benches.

[Gankra]

r=me with benches removed, and commits squashed.

[JeffBelgum]

r=Gankro

[Gankra]

Congrats!

Hope to see more from you in the future. 😄

[mneumann]

I am a bit unhappy about the naming, as it's moving the elements out of the argument. I wouldn't expect that. But I don't have a better name for it...

[Gankra]

This name was accepted in an RFC: rust-lang/rfcs#509

[mneumann]

ok. would have been cleaner to have sth. like move in the name, suggesting that the items are actually moved, but then move_append sounds ugly. I am fine with append, but still it might be unintuitive and someone might expect the argument vector still to contain the values...

[andrewrk]

Why does move_append sound ugly? It describes exactly what the function does.

[mneumann]

@andrewrk: Hm, after thinking about it, yes move_append describes exactly what it does! Given the fact that append in it's current form is an optimization so that you can reuse it's argument, I'd rather go with consuming it's argument append(&mut self, other: Self) and rename it's current form into the more-dangerous-to-use move_append(&mut self, other: &mut Self).

[mneumann]

Ok, we have push_all which clones and append which moves. Without consulting the documentation it's not obvious to me which one does which.

[Gankra]

basically nothing in Rust actually uses the term move

[Gankra]

And push_all is a non-normative performance hack

[huonw]

basically nothing in Rust actually uses the term move

(There's move || ... closures.)

[mneumann]

@gankro: In the past there was move_iter() IIRC which has been renamed to into_iter(). I am still living in the past :D