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

Implemented drain_filter for HashMap #135

Merged
merged 4 commits into from Dec 24, 2019
Merged

Implemented drain_filter for HashMap #135

merged 4 commits into from Dec 24, 2019

Conversation

@JulianKnodt
Copy link
Contributor

JulianKnodt commented Dec 15, 2019

#134
Drain filter is as described in the doc comments.

The implementation is based almost entirely on retain's implementation, as per amanieu's
suggestion.
I messed around with the lifetimes, as I'm not entirely familiar with the unsafe iter
on the raw table, but since we're now using a lazy iterator, the predicate must be valid for as
long as the borrow on the table.

I also annotated the function with a #[must_use], otherwise the drain would have no effect.

Please let me know if there are any other additions before this change can be added.
Thanks!

Edit:
I also realize this could be added to hashset, let me know if I should add that as well,
and if there are other tests that need to be updated.

@JulianKnodt JulianKnodt changed the title #134 Implemented drain_filter #134 Implemented drain_filter for HashMap Dec 15, 2019
@JulianKnodt JulianKnodt force-pushed the JulianKnodt:master branch 3 times, most recently from 97f74b0 to d5f0b29 Dec 16, 2019
@JulianKnodt JulianKnodt changed the title #134 Implemented drain_filter for HashMap Implemented drain_filter for HashMap Dec 16, 2019
Copy link
Collaborator

Amanieu left a comment

My main concern with this implementation is that the behavior of the returned iterator doesn't match Vec::drain_filter or LinkedList::drain_filter when dropped. It should continue running the predicate function on all remaining elements instead of just leaving unprocessed elements in the table.

src/map.rs Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
@JulianKnodt

This comment has been minimized.

Copy link
Contributor Author

JulianKnodt commented Dec 16, 2019

Would I have to add a separate type with its own drop implementation?

@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Dec 16, 2019

Yes a separate DrainFilter iterator type will be needed.

@JulianKnodt JulianKnodt force-pushed the JulianKnodt:master branch 3 times, most recently from 984fd53 to 66bda4e Dec 16, 2019
@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Dec 16, 2019

Can you add a drop guard like in rust-lang/rust#67290.

After that it should be good to go.

@JulianKnodt

This comment has been minimized.

Copy link
Contributor Author

JulianKnodt commented Dec 16, 2019

I've added the guard, but will wait until the embedded stuff gets fixed

@JulianKnodt JulianKnodt force-pushed the JulianKnodt:master branch from c67f0ed to 9598fc8 Dec 17, 2019
@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Dec 21, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2019

📌 Commit 9598fc8 has been approved by Amanieu

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2019

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout master (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self master --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/map.rs
CONFLICT (content): Merge conflict in src/map.rs
Automatic merge failed; fix conflicts and then commit the result.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2019

☔️ The latest upstream changes (presumably #132) made this pull request unmergeable. Please resolve the merge conflicts.

JulianKnodt added 4 commits Dec 15, 2019
Drain filter is as described in the doc comments.

The implementation is based almost entirely on retain's implementation, as per `amanieu`'s
suggestion.
I messed around with the lifetimes, as I'm not entirely familiar with the unsafe `iter`
on the raw table, but since we're now using a lazy iterator, the predicate must be valid for as
long as the borrow on the table.

I also annotated the function with a `#[must_use]`, otherwise the drain would have no effect.

Please let me know if there are any other additions before this change can be added.
Thanks!
Fixed clippy lint (added tab and spaced out `=(K,V)` => `= (K, V)`).

Also made the test more explicit.

Edit:
I've finally installed clippy rather than checking Travis.
The previous implementation did not match vec's drain filter's drop semantics. As per
`amanieu`'s suggestion, added a DrainFilter which implements drop so that it removes the
items which don't satisfy the predicate.
@JulianKnodt JulianKnodt force-pushed the JulianKnodt:master branch from 9598fc8 to 0a65d86 Dec 21, 2019
@JulianKnodt

This comment has been minimized.

Copy link
Contributor Author

JulianKnodt commented Dec 24, 2019

Is this good to go?

@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Dec 24, 2019

Sorry, I don't get email notifications when you force-push.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2019

📌 Commit 0a65d86 has been approved by Amanieu

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2019

⌛️ Testing commit 0a65d86 with merge 11368d0...

bors added a commit that referenced this pull request Dec 24, 2019
Implemented drain_filter for HashMap

#134
Drain filter is as described in the doc comments.

The implementation is based almost entirely on retain's implementation, as per `amanieu`'s
suggestion.
I messed around with the lifetimes, as I'm not entirely familiar with the unsafe `iter`
on the raw table, but since we're now using a lazy iterator, the predicate must be valid for as
long as the borrow on the table.

I also annotated the function with a `#[must_use]`, otherwise the drain would have no effect.

Please let me know if there are any other additions before this change can be added.
Thanks!

Edit:
I also realize this could be added to hashset, let me know if I should add that as well,
and if there are other tests that need to be updated.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2019

☀️ Test successful - checks-travis
Approved by: Amanieu
Pushing 11368d0 to master...

@bors bors merged commit 0a65d86 into rust-lang:master Dec 24, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.