-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Allow replacing HashMap entries #44278
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (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. |
src/libstd/collections/hash/map.rs
Outdated
/// | ||
/// ``` | ||
#[stable(feature = "rust1", since = "1.20.0")] | ||
pub fn replace(mut self, value: V) -> (K, V) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new API should be #[unstable]
.
Why not apply this to |
@kennytm It is non-trivial to implement it for Not sure how to implement recovery of a |
I am quite inexperienced with the std, and I am unsure how such an unstable attribute would look. Adding
|
@Binero You would first need to pick a name of the feature, say The syntax of #[unstable(feature = "entry_replace", reason = "recently added", issue = "44286")] The doctest examples will need to be modified to include /// # Examples
///
/// ```
/// #![feature(entry_replace)]
/// # fn main() {
/// use std::collections::HashMap;
/// use std::collections::hash_map::Entry;
/// // ...
/// # }
/// ```
|
Not sure why this is waiting on author? |
@Binero I'm not sure either. Maybe because of the test failure? It looks like an osx worker had a network failure though; I've restarted that job. I think this is ready for review again @BurntSushi ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few nits, and mechanically, this looks fine to me.
Is this something we definitely want though? Do you have a use case in mind where this method is useful?
src/libstd/collections/hash/map.rs
Outdated
/// # Examples | ||
/// | ||
/// ``` | ||
/// # #![feature(map_entry_replace)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we include the feature toggle in the docs so that end users know that it is necessary. So I'd remove the #
prefix here.
src/libstd/collections/hash/map.rs
Outdated
/// } | ||
/// | ||
/// assert_eq!(map.get("poneyland"), Some(&16)); | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove blank line?
src/libstd/collections/hash/map.rs
Outdated
/// use std::collections::hash_map::Entry; | ||
/// | ||
/// let mut map: HashMap<String, u32> = HashMap::new(); | ||
/// map.insert(String::from("poneyland"), 15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use "poneyland".to_string()
?
src/libstd/collections/hash/map.rs
Outdated
/// let mut map: HashMap<String, u32> = HashMap::new(); | ||
/// map.insert(String::from("poneyland"), 15); | ||
/// | ||
/// if let Entry::Occupied(entry) = map.entry(String::from("poneyland")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, use to_string
.
@BurntSushi The Entry-API serves as a temporary result, to prevent having to lookup entries in the HashMap multiple times. Right now, to get a key out of a
With this addition, the process is simplified to the following:
|
pub fn replace(mut self, value: V) -> (K, V) { | ||
let (old_key, old_value) = self.elem.read_mut(); | ||
|
||
let old_key = mem::replace(old_key, self.key.unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One can take the key out of the entry using take_key
before replace
, so this key.unwrap()
can fail. It needs a custom message at least.
I think this (sort of) already possible using The difference is that |
@arthurprs |
Indeed, thanks for clarifying. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@Binero That's good enough for me! Thanks for clarifying. Should this get an entry in the unstable book before merging? |
@BurntSushi oh we typically don't have an entry per unstable library API as the docs here typically suffice for that, in which case I'll.. @bors: r=BurntSushi Thanks for the PR @Binero! |
📌 Commit d3de465 has been approved by |
Allow replacing HashMap entries This is an obvious API hole. At the moment the only way to retrieve an entry from a `HashMap` is to get an entry to it, remove it, and then insert a new entry. This PR allows entries to be replaced.
☀️ Test successful - status-appveyor, status-travis |
This is an obvious API hole. At the moment the only way to retrieve an entry from a
HashMap
is to get an entry to it, remove it, and then insert a new entry. This PR allows entries to be replaced.