-
Notifications
You must be signed in to change notification settings - Fork 13.9k
add extend_front to VecDeque with specialization like extend #146861
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
base: master
Are you sure you want to change the base?
Conversation
|
cc @the8472 a bunch of questions re: specialization that I believe you may be able to address |
This comment has been minimized.
This comment has been minimized.
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.
Why are certain things behind feature gates, cfg(not(test))
This is due to tests living in the alloctest crate and some of the tests need access to implementation details so they're done with #[path] module imports there which means the originals need to be disabled.
See #136642
cfg(not(no_global_oom_handling)) like
This one is an unstable feature previously used by the linux kernel. IIRC a few other projects are using it too. It's supposed to remove all methods that abort on oom rather than returning an error from alloc's API surface.
Feature was requested in #69939
It's recommended to file an API change proposal to get some preliminary review of the proposed API from the team.
Considering that there are open questions how the API should look like I think it'd be best to have that discussion on an ACP thread so we can focus on the implementation details there.
| } | ||
|
|
||
| #[cfg(not(test))] | ||
| impl<T, A: Allocator> SpecExtendFront<T, Rev<vec::IntoIter<T>>> for VecDeque<T, A> { |
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.
This is a curious specialization. Is the expectation that people will frequently use Rev to prepend to a vecdeque?
Anyway, specializations should have dedicated tests since they're not covered by the regular ones.
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.
This is a curious specialization. Is the expectation that people will frequently use
Revto prepend to a vecdeque?
This depends on the design of extend_front, if it will act like repeated push_front, specializing this can have significant performance improvements. This would be used when prepending elements from a Vec into a VecDeque, in original order, a bit like the requested VecDeque::prepend (#69939 (comment)).
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.
prepend (see tracking issue) will use this, it calls self.extend_front(other.into_iter().rev())
I don't think any of the questions are specific to specialization? |
This comment has been minimized.
This comment has been minimized.
a0a54e3 to
6da1b60
Compare
This comment has been minimized.
This comment has been minimized.
5c9b311 to
d4ae120
Compare
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.
I'm not sure who to ping about the specialisation issue. Apart from that, this seems fine!
|
☔ The latest upstream changes (presumably #148337) made this pull request unmergeable. Please resolve the merge conflicts. |
d4ae120 to
e23c155
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
4eb54f5 to
5b96677
Compare
|
Thank you! |
…r=joboet add extend_front to VecDeque with specialization like extend ACP: rust-lang/libs-team#658 Tracking issue: rust-lang#146975 _Text below was written before opening the ACP_ Feature was requested in rust-lang#69939, I recently also needed it so decided to implement it as my first contribution to the Rust standard library. I plan on doing more but wanted to start with a small change. Some questions I had (both on implementation and design) with answers: - Q: `extend` allows iterators that yield `&T` where `T` is `Clone`, should extend_front do too? A: No, users can use [`copied`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.copied) and/or [`cloned`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.cloned). - Q: Does this need a whole new trait like Extend or only a method on `VecDeque`? A: No, see ACP. - Q: How do I deal with all the code duplication? Most code is similar to that of `extend`, maybe there is a nice way to factor out the code around `push_unchecked`/`push_front_unchecked`. Will come back to this later. - Q: Why are certain things behind feature gates, `cfg(not(test))` like `vec::IntoIter` here and `cfg(not(no_global_oom_handling))` like `Vec::extend_from_within`? (I am also looking at implementing `VecDeque::extend_from_within`) A: See rust-lang#146861 (review) - Q: Should `extend_front` act like repeated pushes to the front of the queue? This reverses the order of the elements. Doing it different might incur an extra move if the iterator length is not known up front (where do you start placing elements in the buffer?). A: `extend_front` acts like repeated pushes, `prepend` preserves the element order, see ACP or tracking issue.
…r=joboet add extend_front to VecDeque with specialization like extend ACP: rust-lang/libs-team#658 Tracking issue: rust-lang#146975 _Text below was written before opening the ACP_ Feature was requested in rust-lang#69939, I recently also needed it so decided to implement it as my first contribution to the Rust standard library. I plan on doing more but wanted to start with a small change. Some questions I had (both on implementation and design) with answers: - Q: `extend` allows iterators that yield `&T` where `T` is `Clone`, should extend_front do too? A: No, users can use [`copied`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.copied) and/or [`cloned`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.cloned). - Q: Does this need a whole new trait like Extend or only a method on `VecDeque`? A: No, see ACP. - Q: How do I deal with all the code duplication? Most code is similar to that of `extend`, maybe there is a nice way to factor out the code around `push_unchecked`/`push_front_unchecked`. Will come back to this later. - Q: Why are certain things behind feature gates, `cfg(not(test))` like `vec::IntoIter` here and `cfg(not(no_global_oom_handling))` like `Vec::extend_from_within`? (I am also looking at implementing `VecDeque::extend_from_within`) A: See rust-lang#146861 (review) - Q: Should `extend_front` act like repeated pushes to the front of the queue? This reverses the order of the elements. Doing it different might incur an extra move if the iterator length is not known up front (where do you start placing elements in the buffer?). A: `extend_front` acts like repeated pushes, `prepend` preserves the element order, see ACP or tracking issue.
…r=joboet add extend_front to VecDeque with specialization like extend ACP: rust-lang/libs-team#658 Tracking issue: rust-lang#146975 _Text below was written before opening the ACP_ Feature was requested in rust-lang#69939, I recently also needed it so decided to implement it as my first contribution to the Rust standard library. I plan on doing more but wanted to start with a small change. Some questions I had (both on implementation and design) with answers: - Q: `extend` allows iterators that yield `&T` where `T` is `Clone`, should extend_front do too? A: No, users can use [`copied`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.copied) and/or [`cloned`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.cloned). - Q: Does this need a whole new trait like Extend or only a method on `VecDeque`? A: No, see ACP. - Q: How do I deal with all the code duplication? Most code is similar to that of `extend`, maybe there is a nice way to factor out the code around `push_unchecked`/`push_front_unchecked`. Will come back to this later. - Q: Why are certain things behind feature gates, `cfg(not(test))` like `vec::IntoIter` here and `cfg(not(no_global_oom_handling))` like `Vec::extend_from_within`? (I am also looking at implementing `VecDeque::extend_from_within`) A: See rust-lang#146861 (review) - Q: Should `extend_front` act like repeated pushes to the front of the queue? This reverses the order of the elements. Doing it different might incur an extra move if the iterator length is not known up front (where do you start placing elements in the buffer?). A: `extend_front` acts like repeated pushes, `prepend` preserves the element order, see ACP or tracking issue.
…r=joboet add extend_front to VecDeque with specialization like extend ACP: rust-lang/libs-team#658 Tracking issue: rust-lang#146975 _Text below was written before opening the ACP_ Feature was requested in rust-lang#69939, I recently also needed it so decided to implement it as my first contribution to the Rust standard library. I plan on doing more but wanted to start with a small change. Some questions I had (both on implementation and design) with answers: - Q: `extend` allows iterators that yield `&T` where `T` is `Clone`, should extend_front do too? A: No, users can use [`copied`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.copied) and/or [`cloned`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.cloned). - Q: Does this need a whole new trait like Extend or only a method on `VecDeque`? A: No, see ACP. - Q: How do I deal with all the code duplication? Most code is similar to that of `extend`, maybe there is a nice way to factor out the code around `push_unchecked`/`push_front_unchecked`. Will come back to this later. - Q: Why are certain things behind feature gates, `cfg(not(test))` like `vec::IntoIter` here and `cfg(not(no_global_oom_handling))` like `Vec::extend_from_within`? (I am also looking at implementing `VecDeque::extend_from_within`) A: See rust-lang#146861 (review) - Q: Should `extend_front` act like repeated pushes to the front of the queue? This reverses the order of the elements. Doing it different might incur an extra move if the iterator length is not known up front (where do you start placing elements in the buffer?). A: `extend_front` acts like repeated pushes, `prepend` preserves the element order, see ACP or tracking issue.
ACP: rust-lang/libs-team#658
Tracking issue: #146975
Text below was written before opening the ACP
Feature was requested in #69939, I recently also needed it so decided to implement it as my first contribution to the Rust standard library. I plan on doing more but wanted to start with a small change.
Some questions I had (both on implementation and design) with answers:
extendallows iterators that yield&TwhereTisClone, should extend_front do too?A: No, users can use
copiedand/orcloned.VecDeque?A: No, see ACP.
extend, maybe there is a nice way to factor out the code aroundpush_unchecked/push_front_unchecked.Will come back to this later.
cfg(not(test))likevec::IntoIterhere andcfg(not(no_global_oom_handling))likeVec::extend_from_within? (I am also looking at implementingVecDeque::extend_from_within)A: See add extend_front to VecDeque with specialization like extend #146861 (review)
extend_frontact like repeated pushes to the front of the queue? This reverses the order of the elements. Doing it different might incur an extra move if the iterator length is not known up front (where do you start placing elements in the buffer?).A:
extend_frontacts like repeated pushes,prependpreserves the element order, see ACP or tracking issue.