-
Notifications
You must be signed in to change notification settings - Fork 131
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 ArrayVec.extend_from_slice and capacity_left #101
Conversation
I was just about to make a PR introducing exactly this. @bluss are there any things preventing this PR from being merged? |
I think this is a good change. Can we add to the extend benchmarks, and demonstrate that this is worthwhile? Also, please don't include the formatting and whitespace changes in the PR. |
I've rebased on master, removed the cleanup commit and add a benchmark, for which I got the following results:
This is only a single run on a laptop, so take it with a grain of salt but I think the results are clear. |
I also should note I'm not sure if I used |
Yes very nice. See my linked pr which should improve regular extend. But I think we can reuse std's reasoning here, and they haven't deprecated this method yet. |
@bluss so is this acceptable to merge, or would you like to see some more changes? |
It's good, just thinking of the name of capacity left, does it have a precedent in std or otherwise? Maybe a name like remaining_capacity? Also the alternative of not exposing it. Also the panic is a point of API decision but I think it is fine -- but it has different semantics than extend which is problematic |
For the As for the panicing in /// Extend the `ArrayVec` from `other`. Returns the number of elements copied.
pub fn extend_from_slice(&mut self, other: &[A::Item]) -> usize
where A::Item: Copy,
{
let count = ::std::cmp::min(self.capacity_left(), other.len());
if count == 0 {
return 0;
}
// Copy `count` elements ...
count
} |
Ok, I think we can merge this. I can apply the following comments on my own, so that we stop the ping-ponging:
See also PR #112 which makes the regular |
Thanks for adding this! |
No, ok, that doesn't feel right.
I think this should become |
Ok seems good to me. |
Next release will be whenever #97 becomes "unblocked". We have merged several breaking changes, and that's important to get into the next version too. |
This add two new public methods on
ArrayVec
:extend_from_slice
: extends the vector with a slice using a memcpy.capacity_left
: utility function for use withextend_from_slice
, e.g. see fa4e22f.This also changes the
Write
implementation to useextend_from_slice
and another commit that removes some whitespace and empty lines.