Skip to content
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

Merged
merged 4 commits into from
Mar 5, 2015
Merged

Replace Vec::drain by a method that accepts a range parameter. #574

merged 4 commits into from
Mar 5, 2015

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Jan 12, 2015

@ftxqxd
Copy link
Contributor

ftxqxd commented Jan 13, 2015

I’m probably being stupid, but why can’t similar versions of these methods be implemented for String as well (presumably yielding chars)?

@mahkoh
Copy link
Contributor Author

mahkoh commented Jan 13, 2015

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 str::slice(...).chars(); str::remove_range(...) because everything is Copy. remove_range comes from #570.

@ftxqxd
Copy link
Contributor

ftxqxd commented Jan 13, 2015

You might not be able to share an implementation, but surely you could still have the new & improved drain method on String, even if its implementation were different? The API would be more consistent that way, in my opinion, and drain with a range parameter is strictly more useful than a remove_range method, even on a string, where chars are Copy.

@mahkoh
Copy link
Contributor Author

mahkoh commented Jan 13, 2015

A drain would not provide new functionality, would be slower in unoptimized builds, and would make compile times longer. A remove_range function on String is orthogonal to the current set of methods.

It's unfortunate that drain on Vec will also have these problems but I don't think we can add both drain and remove_range.

@aturon aturon self-assigned this Jan 22, 2015
@aturon
Copy link
Member

aturon commented Feb 12, 2015

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?

@mahkoh
Copy link
Contributor Author

mahkoh commented Feb 13, 2015

I've updated the RFC. Details are left to the implementation.

@bombless
Copy link

Seems good.
Is it possible to take Iterator as range?
Say, we add + and == to Iterator, and make its Output like an old-school struct Drain that impls Iterator.
I'm trying to figure out the lifetime thing for this.

@huonw
Copy link
Member

huonw commented Feb 13, 2015

There's no drawbacks to this design?

@mahkoh
Copy link
Contributor Author

mahkoh commented Feb 13, 2015

None. Perfect RFC.

At least none that I'm aware of. If you have some I can add them.

@huonw
Copy link
Member

huonw commented Feb 13, 2015

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 drain method to differ between Vec/String and other types.

/// 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: /* ? */) -> /* ? */ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these omitted?

Copy link
Contributor Author

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.

Copy link
Member

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.

@mahkoh
Copy link
Contributor Author

mahkoh commented Feb 13, 2015

Updated with drawbacks.

@aturon
Copy link
Member

aturon commented Feb 17, 2015

@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 Drainer trait, I'd prefer to add a general trait for abstracting over all range type. (This request has already come up in the past). You could update the RFC text to use such a trait, but it's fine if you don't spell out the full details.

Otherwise, I think this is ready to go.

@mahkoh
Copy link
Contributor Author

mahkoh commented Feb 18, 2015

I've updated the detailed design.

@aturon
Copy link
Member

aturon commented Feb 19, 2015

@mahkoh, thanks, I've added this to the agenda for reaching a final decision.

@aturon
Copy link
Member

aturon commented Mar 5, 2015

After discussion on the core team and with stakeholders here and on IRC, this RFC has been approved.

Tracking issue

@Gankra
Copy link
Contributor

Gankra commented Apr 15, 2015

drain_range is apparently unsound due to safe code apparently not being able to guarantee drop being called. That is mem::forget is strictly speaking safe to implement and use.

bors added a commit to rust-lang/rust that referenced this pull request Apr 28, 2015
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
@Centril Centril added the A-collections Proposals about collection APIs label Nov 23, 2018
@Centril Centril added the A-ranges Proposals relating to ranges. label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Proposals about collection APIs A-ranges Proposals relating to ranges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants