Implement (HashMap) Entry::insert as per #60142 #64656
Merged
+66
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
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 |
dtolnay
reviewed
Sep 29, 2019
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.
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 |
Dylan-DPC-zz
added
S-waiting-on-author
|
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. |
bors
added a commit
to rust-lang/hashbrown
that referenced
this pull request
Oct 4, 2019
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.
Dylan-DPC-zz
added
S-blocked
|
I just published hashbrown 0.6.1 with your patch. Now you need to update hashbrown in Cargo.lock in this PR. |
rustbot
added
the
S-waiting-on-review
|
Can you create a tracking issue and point to it in the |
4 tasks
|
@Amanieu all done |
|
@bors r+ |
|
📌 Commit bdcc21c has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit bdcc21c with merge 54ff3e75f4ac76ce517a58d221d55cda6096b47d... |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 9, 2019
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. |
bors
added a commit
that referenced
this pull request
Oct 9, 2019
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... |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 9, 2019
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. |
bors
added a commit
that referenced
this pull request
Oct 9, 2019
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
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Dec 21, 2021
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.
fmease
added a commit
to fmease/rust
that referenced
this pull request
Sep 14, 2024
…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.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 14, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation of
Entry::insertas 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_returnsuch 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.)