-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Replace Vec::drain
by a method that accepts a range parameter.
#574
Conversation
I’m probably being stupid, but why can’t similar versions of these methods be implemented for |
You can't use the same implementation as the Vec one because the chars are not stored as chars. You can already achieve this by calling |
You might not be able to share an implementation, but surely you could still have the new & improved |
A It's unfortunate that |
I like the idea of this RFC as well as the one for `String, but like @P1start would strongly prefer for the APIs to match. @mahkoh, can you provide any evidence that the performance hit on debug builds is bad enough to merit API inconsistency at this stage? |
I've updated the RFC. Details are left to the implementation. |
Seems good. |
There's no drawbacks to this design? |
None. Perfect RFC. At least none that I'm aware of. If you have some I can add them. |
There's at least the obvious one of making things more generic and hence the documentation harder to use. It also causes the API for the |
/// Panics if the range is decreasing, if the upper bound is larger than the | ||
/// length of the String, or if the start and the end of the range don't lie on | ||
/// character boundaries. | ||
pub fn drain(&mut self, range: /* ? */) -> /* ? */ { |
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 these omitted?
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.
Assuming that the return value can only be constructed inside the String module, these parts of the signature are merely an implementation detail. If you want to you can assume
pub fn drain<T: StringDrainer>(&mut self, range: T) -> CharRangeIter<T> {
or something similar.
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 public signature doesn't seem like an implementation detail to me.
Updated with drawbacks. |
@mahkoh Thanks for the revisions, this definitely feels better to me. Would you mind filling in the details that @huonw requested? Also, rather than a Otherwise, I think this is ready to go. |
I've updated the detailed design. |
@mahkoh, thanks, I've added this to the agenda for reaching a final decision. |
After discussion on the core team and with stakeholders here and on IRC, this RFC has been approved. |
drain_range is apparently unsound due to safe code apparently not being able to guarantee |
Implement Vec::drain(\<range type\>) from rust-lang/rfcs#574, tracking issue #23055. This is a big step forward for vector usability. This is an introduction of an API for removing a range of *m* consecutive elements from a vector, as efficently as possible. New features: - Introduce trait `std::collections::range::RangeArgument` implemented by all four built-in range types. - Change `Vec::drain()` to use `Vec::drain<R: RangeArgument>(R)` Implementation notes: - Use @gankro's idea for memory safety: Use `set_len` on the source vector when creating the iterator, to make sure that the part of the vector that will be modified is unreachable. Fix up things in Drain's destructor — but even if it doesn't run, we don't expose any moved-out-from slots of the vector. - This `.drain<R>(R)` very close to how it is specified in the RFC. - Introduced as unstable - Drain reuses the slice iterator — copying and pasting the same iterator pointer arithmetic again felt very bad - The `usize` index as a range argument in the RFC is not included. The ranges trait would have to change to accomodate it. Please help me with: - Name and location of the new ranges trait. - Design of the ranges trait - Understanding Niko's comments about variance (Note: for a long time I was using a straight up &mut Vec in the iterator, but I changed this to permit reusing the slice iterator). Previous PR and discussion: #23071
Rendered