-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add extend_from_array #79892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add extend_from_array #79892
Conversation
(Hmm, that codegen test passed locally. I'll leave it for now, and fix it along with adding a tracking issue if this gets a "seems plausible".) |
Co-authored-by: erikdesjardins <[email protected]>
Thanks, @erikdesjardins! That sounds plausible. And because that means this might fail in bors even after passing CI, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good to me, but once IntoIterator
is implemented for arrays (#65819), v.extend_from_array([1, 2, 3])
would be equivalent to just v.extend([1, 2, 3])
, making this new api redundant, right? (Maybe with a specialization or two in its implementation.)
unsafe { | ||
let end = self.as_mut_ptr().add(self.len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a safe way to get the end pointer: self.as_mut_ptr_range().end
☔ The latest upstream changes (presumably #80138) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
I'm not going to be able to look at this for a bit, with edition stuff being higher priority. I'll close for now, and maybe will reopen as not a trait method in the future. |
This PR inspired by noticing that the following, while it seems reasonable, actually contains 4 calls to
reserve
in the ASM:https://rust.godbolt.org/z/EWExYP
Like there's
extend_from_slice
for cloning, it seems like it would be logical to have anextend_from_array
to encapsulate the obviousunsafe
code for moving the elements to the end of the vector. And, indeed, that turns out to generate tighter assembly than any of the other options I tried, includingarray::IntoIter
andchain
s-of-once
s: https://rust.godbolt.org/z/Wq7qahI ended up putting this on
Extend
, as it makes particular sense on anything that can take advantage of the memory continuity of the array -- this PR implements it forVec
andVecDeque
-- but would also be handy as a way to tersely add a known number of elements to any collection by move. It has the nice default implementation in terms of the arrayIntoIter
, so that things without special overrides for it can still do something reasonable, as that provides an exactsize_hint
.But touching a trait means it should probably get some libs eyes on it even to go in unstable, so let's try
r? @m-ou-se
(A different approach for this could be something like
push_chunk
, in the sense ofas_chunks
.)