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

Introduce LinkedList::drain_filter #46262

Merged
merged 2 commits into from Nov 28, 2017

Conversation

Projects
None yet
6 participants
@udoprog
Contributor

udoprog commented Nov 25, 2017

This introduces LinkedList::remove_if.

This operation embodies one of the use-cases where LinkedList would typically be preferred over Vec: random removal and retrieval.

There are a number of considerations with this:

Should there be two remove_if methods? One where elements are only removed, one which returns a collection of removed elements.

Should this be implemented using a draining iterator pattern that covers both cases? I suspect that would incur a bit of overhead (moving the element into the iterator, then into a new collection). But I'm not sure. Maybe that's an acceptable compromise to maximize flexibility.

I don't feel I've had enough exposure to unsafe programming in rust to be certain the implementation is correct. This relies quite heavily on moving around copies of Shared pointers to make the code reasonable. Please help me out :).

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 25, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Nov 25, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 25, 2017

Member

This PR introduces a new method. Deferring the decision to the libs team.

r? @dtolnay

Member

kennytm commented Nov 25, 2017

This PR introduces a new method. Deferring the decision to the libs team.

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm Nov 25, 2017

@dtolnay

Vec::drain_filter is a much more general and equally efficient form of remove_if where the caller can do whatever they want with the removed values (building a new LinkedList of them, as with remove_if, seems like an uncommon use case). The equivalent of your example code would be:

let mut d = LinkedList::new();
d.push_back(1);
d.push_back(2);
d.push_back(3);

let mut removed = LinkedList::new();
for node in d.drain_filter(|v| *v < 3) {
    removed.push_back(node);
}
assert_eq!(removed.len(), 2);
assert_eq!(d.len(), 1);

I would prefer to be consistent by providing the same API for LinkedList.

@udoprog

This comment has been minimized.

Show comment
Hide comment
@udoprog

udoprog Nov 25, 2017

Contributor

@dtolnay will do, that also answers most of my questions. Thanks!

Contributor

udoprog commented Nov 25, 2017

@dtolnay will do, that also answers most of my questions. Thanks!

@udoprog

This comment has been minimized.

Show comment
Hide comment
@udoprog

udoprog Nov 25, 2017

Contributor

Relates to rust-lang/rfcs#2140

Contributor

udoprog commented Nov 25, 2017

Relates to rust-lang/rfcs#2140

Implement LinkedList::drain_filter
Relates to rust-lang/rfcs#2140 - drain_filter for all collections

`drain_filter` is implemented instead of `LinkedList::remove_if` based
on review feedback.
@udoprog

This comment has been minimized.

Show comment
Hide comment
@udoprog

udoprog Nov 25, 2017

Contributor

I ported all the tests on Vec for drain_filter. Would be nice to abstract them, but I felt that would be a change that is a bit too large for my taste.

All changes are now also associated with the drain_filter feature.

Contributor

udoprog commented Nov 25, 2017

I ported all the tests on Vec for drain_filter. Would be nice to abstract them, but I felt that would be a change that is a bit too large for my taste.

All changes are now also associated with the drain_filter feature.

@udoprog udoprog changed the title from Introduce LinkedList::remove_if to Introduce LinkedList::drain_filter Nov 25, 2017

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Nov 25, 2017

Contributor

Consistency is important. Note however that Linked List can do this kind of operation -- remove some elements during the scan and string them together a resulting linked list of the removed elements -- without any de/allocation of list nodes or even any moving of element values. It would be good to expose that functionality as well, but it is kind of its own kind of operation.

Contributor

bluss commented Nov 25, 2017

Consistency is important. Note however that Linked List can do this kind of operation -- remove some elements during the scan and string them together a resulting linked list of the removed elements -- without any de/allocation of list nodes or even any moving of element values. It would be good to expose that functionality as well, but it is kind of its own kind of operation.

@udoprog

This comment has been minimized.

Show comment
Hide comment
@udoprog

udoprog Nov 25, 2017

Contributor

The original patch did something like that. It could at some point be resurrected as a method on DrainFilter if it's deemed important.

I'd be curious to see how well the optimizer deals with the example provided by @dtolnay. I wouldn't be surprised if specialized methods aren't needed.

Contributor

udoprog commented Nov 25, 2017

The original patch did something like that. It could at some point be resurrected as a method on DrainFilter if it's deemed important.

I'd be curious to see how well the optimizer deals with the example provided by @dtolnay. I wouldn't be surprised if specialized methods aren't needed.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Nov 26, 2017

Contributor

This could be covered with specialization. Imagine LinkedList::extend(into iter) and LinkedList::extend(drain_filter) (extend brings collect with it implicitly) both can get this specialization, to just reuse the list nodes. It's got an appeal of not needing any additional API design and it hits consistency as well.

Contributor

bluss commented Nov 26, 2017

This could be covered with specialization. Imagine LinkedList::extend(into iter) and LinkedList::extend(drain_filter) (extend brings collect with it implicitly) both can get this specialization, to just reuse the list nodes. It's got an appeal of not needing any additional API design and it hits consistency as well.

@udoprog

This comment has been minimized.

Show comment
Hide comment
@udoprog

udoprog Nov 27, 2017

Contributor

Since the extend approach is compatible with a draining iterator it can be introduced separately.

@dtolnay With this in mind, can I get another round of review?

Contributor

udoprog commented Nov 27, 2017

Since the extend approach is compatible with a draining iterator it can be introduced separately.

@dtolnay With this in mind, can I get another round of review?

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay
Member

dtolnay commented Nov 27, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 27, 2017

Contributor

📌 Commit 60aa834 has been approved by dtolnay

Contributor

bors commented Nov 27, 2017

📌 Commit 60aa834 has been approved by dtolnay

kennytm added a commit to kennytm/rust that referenced this pull request Nov 27, 2017

Rollup merge of rust-lang#46262 - udoprog:linked-list-remove-if, r=dt…
…olnay

Introduce LinkedList::drain_filter

This introduces `LinkedList::remove_if`.

This operation embodies one of the use-cases where `LinkedList` would typically be preferred over `Vec`: random removal and retrieval.

There are a number of considerations with this:

Should there be two `remove_if` methods? One where elements are only removed, one which returns a collection of removed elements.

Should this be implemented using a draining iterator pattern that covers both cases? I suspect that would incur a bit of overhead (moving the element into the iterator, then into a new collection). But I'm not sure. Maybe that's an acceptable compromise to maximize flexibility.

I don't feel I've had enough exposure to unsafe programming in rust to be certain the implementation is correct. This relies quite heavily on moving around copies of Shared pointers to make the code reasonable. Please help me out :).

bors added a commit that referenced this pull request Nov 27, 2017

Auto merge of #46312 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45506, #46174, #46231, #46240, #46249, #46258, #46262, #46275, #46282, #46285
- Failed merges:

bors added a commit that referenced this pull request Nov 27, 2017

Auto merge of #46312 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45506, #46174, #46231, #46240, #46249, #46258, #46262, #46275, #46282, #46285
- Failed merges:

@bors bors merged commit 60aa834 into rust-lang:master Nov 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@udoprog udoprog deleted the udoprog:linked-list-remove-if branch Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment