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

Tracking issue for Replace Vec::drain by a method that accepts a range parameter (RFC 574) #23055

Closed
aturon opened this issue Mar 5, 2015 · 3 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Mar 5, 2015

rust-lang/rfcs#574

cc @mahkoh

@aturon aturon added A-libs B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. labels Mar 5, 2015
bors added a commit that referenced this issue 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
bluss pushed a commit to bluss/rust that referenced this issue May 1, 2015
`.drain(range)` is unstable and under feature(collections_drain).

This adds a safe way to remove any range of a String as efficiently as
possible.

As noted in the code, this drain iterator has none of the memory safety
issues of the vector version.

RFC tracking issue is rust-lang#23055
bluss pushed a commit to bluss/rust that referenced this issue May 1, 2015
`.drain(range)` is unstable and under feature(collections_drain).

This adds a safe way to remove any range of a String as efficiently as
possible.

As noted in the code, this drain iterator has none of the memory safety
issues of the vector version.

RFC tracking issue is rust-lang#23055
bluss pushed a commit to bluss/rust that referenced this issue May 1, 2015
`.drain(range)` is unstable and under feature(collections_drain).

This adds a safe way to remove any range of a String as efficiently as
possible.

As noted in the code, this drain iterator has none of the memory safety
issues of the vector version.

RFC tracking issue is rust-lang#23055
bors added a commit that referenced this issue May 2, 2015
collections: Implement String::drain(range) according to RFC 574

`.drain(range)` is unstable and under feature(collections_drain).

This adds a safe way to remove any range of a String as efficiently as
possible.

As noted in the code, this drain iterator has none of the memory safety
issues of the vector version.

RFC tracking issue is #23055
@paulp
Copy link

paulp commented Jun 14, 2015

The rfc says

Where Trait is some trait that is implemented for at least Range, RangeTo, RangeFrom, FullRange, and usize.

It also includes an example which depends on usize:

fn remove(x: &mut Vec<u8>, index: usize) -> u8 {
    x.drain_range(index).next().unwrap()
}

However RangeArgument only implements the first four. Furthermore as things stand it does not appear possible to accept RangeArgument and usize simultaneously, because dispatch doesn't consider the trait bound and reports that conflicting implementations exist.

@bluss
Copy link
Member

bluss commented Jul 13, 2015

@paulp Yes, #24781 was explicit about these issues. I forsee a new RFC to finish off drain and maybe the RangeArgument trait.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 11, 2015
@alexcrichton
Copy link
Member

I believe this has since been implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants