-
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
Implement (HashMap) Entry::insert as per #60142 #64656
Conversation
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/libstd/collections/hash/map.rs
Outdated
/// assert_eq!(map["poneyland"], 37); | ||
/// ``` | ||
#[inline] | ||
#[stable(feature = "rust1", since = "1.0.0")] |
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 line should be removed. I don't think we put stability attributes on private signatures.
The signature of Entry::insert looks good to me. Are there any next steps on the hashbrown side before we can land this? |
Thanks! I'll do the changes here and go open a PR on the hashbrown side with the changeset. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I'm not too happy with the naming here. I feel that calling both methods (in |
Weakly disagree on the public, as what's being inserted is a value, like |
Oh I didn't notice that Regarding the implementation, I think hashbrown's RustcEntry (and Entry) should have an |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Is that possible? In libstd That does feel awkward, though, to be sure. |
Ah good point, I guess that's not possible then. In that case I'm happy to accept it as it is. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r? @Amanieu |
Er, sorry, can I get clarification on what action is required from me (re |
I think the build fail is the reason. |
Okay, this is now waiting on hashbrown release. |
Add RustcVacantEntry::insert_entry for rust-lang/rust#64656 See rust-lang/rust#64656. ~~This was based on v0.5.0 as that's what rustc uses; I can rebase onto master, but I'm not sure whether rustc wants v0.6.0 or if v0.6.0 is rustc-ready.~~ For context, this ultimately provides an API with this shape: ```rust impl Entry<'a, K, V> { fn insert(self, value: V) -> OccupiedEntry<'a, K, V> {…} } ``` to be used when one wants to insert a value without consuming the Entry, e.g. because one wants to keep a reference to the key around. There's more at the original (defunct) PR: rust-lang/rust#60142.
I just published hashbrown 0.6.1 with your patch. Now you need to update hashbrown in Cargo.lock in this PR. |
Can you create a tracking issue and point to it in the |
@Amanieu all done |
@bors r+ |
📌 Commit bdcc21c has been approved by |
⌛ Testing commit bdcc21c with merge 54ff3e75f4ac76ce517a58d221d55cda6096b47d... |
Implement (HashMap) Entry::insert as per rust-lang#60142 Implementation of `Entry::insert` as per @SimonSapin's comment on rust-lang#60142. This requires a patch to hashbrown: ```diff diff --git a/src/rustc_entry.rs b/src/rustc_entry.rs index fefa5c3..7de8300 100644 --- a/src/rustc_entry.rs +++ b/src/rustc_entry.rs @@ -546,6 +546,32 @@ impl<'a, K, V> RustcVacantEntry<'a, K, V> { let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); unsafe { &mut bucket.as_mut().1 } } + + /// Sets the value of the entry with the RustcVacantEntry's key, + /// and returns a RustcOccupiedEntry. + /// + /// # Examples + /// + /// ``` + /// use hashbrown::HashMap; + /// use hashbrown::hash_map::RustcEntry; + /// + /// let mut map: HashMap<&str, u32> = HashMap::new(); + /// + /// if let RustcEntry::Vacant(v) = map.rustc_entry("poneyland") { + /// let o = v.insert_and_return(37); + /// assert_eq!(o.get(), &37); + /// } + /// ``` + #[inline] + pub fn insert_and_return(self, value: V) -> RustcOccupiedEntry<'a, K, V> { + let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); + RustcOccupiedEntry { + key: None, + elem: bucket, + table: self.table + } + } } impl<K, V> IterMut<'_, K, V> { ``` This is also only an implementation for HashMap. I tried implementing for BTreeMap, but I don't really understand BTreeMap's internals and require more guidance on implementing the equivalent `VacantEntry::insert_and_return` such that it returns an `OccupiedEntry`. Notably, following the original PR's modifications I end up needing a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::LeafOrInternal>, _>` while I only have a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::Internal>, _>` and don't know how to proceed. (To be clear, I'm not asking for guidance right now; I'd be happy getting only the HashMap implementation — the subject of this PR — reviewed and ready, and leave the BTreeMap implementation for a latter PR.)
@bors retry rolled up. |
Rollup of 6 pull requests Successful merges: - #64656 (Implement (HashMap) Entry::insert as per #60142) - #64986 (Function pointers as const generic arguments) - #65037 (`#[track_caller]` feature gate (RFC 2091 1/N)) - #65166 (Suggest to add `move` keyword for generator capture) - #65175 (add more info in debug traces for gcu merging) - #65220 (Update LLVM for Emscripten exception handling support) Failed merges: r? @ghost
⌛ Testing commit bdcc21c with merge eef1f5eeb8d8bb8b57919a58795d2f2ad1bcf27c... |
Implement (HashMap) Entry::insert as per rust-lang#60142 Implementation of `Entry::insert` as per @SimonSapin's comment on rust-lang#60142. This requires a patch to hashbrown: ```diff diff --git a/src/rustc_entry.rs b/src/rustc_entry.rs index fefa5c3..7de8300 100644 --- a/src/rustc_entry.rs +++ b/src/rustc_entry.rs @@ -546,6 +546,32 @@ impl<'a, K, V> RustcVacantEntry<'a, K, V> { let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); unsafe { &mut bucket.as_mut().1 } } + + /// Sets the value of the entry with the RustcVacantEntry's key, + /// and returns a RustcOccupiedEntry. + /// + /// # Examples + /// + /// ``` + /// use hashbrown::HashMap; + /// use hashbrown::hash_map::RustcEntry; + /// + /// let mut map: HashMap<&str, u32> = HashMap::new(); + /// + /// if let RustcEntry::Vacant(v) = map.rustc_entry("poneyland") { + /// let o = v.insert_and_return(37); + /// assert_eq!(o.get(), &37); + /// } + /// ``` + #[inline] + pub fn insert_and_return(self, value: V) -> RustcOccupiedEntry<'a, K, V> { + let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); + RustcOccupiedEntry { + key: None, + elem: bucket, + table: self.table + } + } } impl<K, V> IterMut<'_, K, V> { ``` This is also only an implementation for HashMap. I tried implementing for BTreeMap, but I don't really understand BTreeMap's internals and require more guidance on implementing the equivalent `VacantEntry::insert_and_return` such that it returns an `OccupiedEntry`. Notably, following the original PR's modifications I end up needing a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::LeafOrInternal>, _>` while I only have a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::Internal>, _>` and don't know how to proceed. (To be clear, I'm not asking for guidance right now; I'd be happy getting only the HashMap implementation — the subject of this PR — reviewed and ready, and leave the BTreeMap implementation for a latter PR.)
@bors retry rolled up. |
Rollup of 4 pull requests Successful merges: - #64656 (Implement (HashMap) Entry::insert as per #60142) - #65037 (`#[track_caller]` feature gate (RFC 2091 1/N)) - #65166 (Suggest to add `move` keyword for generator capture) - #65175 (add more info in debug traces for gcu merging) Failed merges: r? @ghost
Stabilise entry_insert This stabilises `HashMap:Entry::insert_entry` etc. Tracking issue rust-lang#65225. It will need an FCP. This was implemented in rust-lang#64656 two years ago. This PR includes the rename and change discussed in rust-lang#65225 (comment), happy to split if needed.
…ChrisDenton Stabilize entry_insert This stabilises `HashMap::Entry::insert_entry`, following the FCP in tracking issue rust-lang#65225. This was implemented in rust-lang#64656 five years ago.
Rollup merge of rust-lang#130290 - passcod:stabilise-entry-insert, r=ChrisDenton Stabilize entry_insert This stabilises `HashMap::Entry::insert_entry`, following the FCP in tracking issue rust-lang#65225. This was implemented in rust-lang#64656 five years ago.
Implementation of
Entry::insert
as per @SimonSapin's comment on #60142. This requires a patch to hashbrown:This is also only an implementation for HashMap. I tried implementing for BTreeMap, but I don't really understand BTreeMap's internals and require more guidance on implementing the equivalent
VacantEntry::insert_and_return
such that it returns anOccupiedEntry
. Notably, following the original PR's modifications I end up needing aHandle<NodeRef<marker::Mut<'_>, _, _, marker::LeafOrInternal>, _>
while I only have aHandle<NodeRef<marker::Mut<'_>, _, _, marker::Internal>, _>
and don't know how to proceed.(To be clear, I'm not asking for guidance right now; I'd be happy getting only the HashMap implementation — the subject of this PR — reviewed and ready, and leave the BTreeMap implementation for a latter PR.)