Skip to content

Rename get_each_mut to get_many_mut and align API with the stdlib#291

Merged
Amanieu merged 1 commit intorust-lang:masterfrom
shepmaster:get_many_mut
Nov 7, 2021
Merged

Rename get_each_mut to get_many_mut and align API with the stdlib#291
Amanieu merged 1 commit intorust-lang:masterfrom
shepmaster:get_many_mut

Conversation

@shepmaster
Copy link
Copy Markdown
Member

@shepmaster shepmaster commented Sep 1, 2021 •
edited
Loading

The standard library will be gaining a similar ability for slices. This updates Hashbrown's API to match that PR and use only stable MaybeUninit functions.

This change requires a MSRV bump to Rust 1.51.

Amanieu
Amanieu reviewed Sep 1, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

Could you add get_many_unchecked_mut as well?

Also bump the Rust version in CI to 1.51 so the tests pass.

Comment thread src/map.rs Outdated
@shepmaster shepmaster force-pushed the get_many_mut branch 2 times, most recently from 24d4b19 to 9f2dab8 Compare September 2, 2021 00:24
@shepmaster
Copy link
Copy Markdown
Member Author

shepmaster commented Sep 2, 2021

Could you add get_many_unchecked_mut as well?

Done

Also bump the Rust version in CI to 1.51 so the tests pass.

Done

I also changed the implementation of the raw get_many_mut to avoid looping twice, which should be a bit more performant (but I did not test that).

Amanieu
Amanieu reviewed Sep 2, 2021
View reviewed changes
Comment thread src/map.rs
Comment thread src/raw/mod.rs Outdated
Amanieu
Amanieu reviewed Sep 5, 2021
View reviewed changes
Comment thread src/raw/mod.rs
Comment on lines +857 to +858
/// At most one mutable reference will be returned to any entry. `None` will be returned if any
/// of the hashes are duplicates. `None` will be returned if the hash is not found.
Copy link
Copy Markdown
Member

@Amanieu Amanieu Sep 5, 2021

Choose a reason for hiding this comment

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

Suggested change
/// At most one mutable reference will be returned to any entry. `None` will be returned if any
/// of the hashes are duplicates. `None` will be returned if the hash is not found.
/// At most one mutable reference will be returned to any entry. `None` will be returned if any
/// of the entries are duplicates. `None` will be returned if an entry is not found.

@Amanieu
Copy link
Copy Markdown
Member

Amanieu commented Sep 5, 2021

LGTM, I was waiting for a reply on rust-lang/rust#83608 about returning a Result instead of an Option but I guess we can always change this later if needed.

@bors
Copy link
Copy Markdown
Contributor

bors commented Nov 6, 2021

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

@Amanieu
Copy link
Copy Markdown
Member

Amanieu commented Nov 7, 2021

Sorry, this slipped off my review queue. Could you rebase so I can merge this? The only conflict should be the CI version change.

@shepmaster
Copy link
Copy Markdown
Member Author

shepmaster commented Nov 7, 2021

Rebased and now using array::map as it has stabilized since the original PR was opened.

@shepmaster
Rename get_each_mut to get_many_mut and align API with the stdlib
c9c361a 
The standard library will be gaining a similar [ability for
slices][]. This updates Hashbrown's API to match that PR and use only
stable `MaybeUninit` functions.

[ability for slices]: rust-lang/rust#83608
@Amanieu Amanieu merged commit 462666a into rust-lang:master Nov 7, 2021
@shepmaster shepmaster deleted the get_many_mut branch November 7, 2021 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Amanieu Amanieu Amanieu left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shepmaster @Amanieu @bors