-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
WIP Add Set entry API #120077
base: master
Are you sure you want to change the base?
WIP Add Set entry API #120077
Conversation
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
T: Hash, | ||
S: BuildHasher, | ||
{ | ||
todo!() |
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.
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.
See what @Amanieu thinks as the hashbrown
maintainer.
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 would prefer that is be added as a public API in hashbrown too.
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.
What would be the difference between insert
and insert_entry
? It kind of seems to me like those are the same API? Please correct me if I'm wrong, but otherwise I'd propose deprecating insert
and making insert_entry
public in hashbrown and then just having insert_entry -> OccupiedEntry
in the stdlib.
https://doc.rust-lang.org/std/collections/hash_map/struct.VacantEntry.html#method.insert
The hashmap needs both insert
and insert_entry
because one returns a reference to the value while the other to the entry. But for hashset there is no value, so we don't need insert at all.
Thoughts?
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.
@Amanieu thoughts on the API changes I proposed?
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.
While reviewing the entry API, I noticed a subtle issue. insert_entry
will consume the key in VacantEntry
to insert it into the set. This will result in it returning an OccupiedEntry
with key set to None
. This will then panic if you try to use OccupiedEntry::replace
since there is no search key to replace the original key with.
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 like map has the same issue: https://github.com/rust-lang/hashbrown/blob/4c824c548e85d75f95e0dfcee09c62c5203f9a75/src/map.rs#L5702
Shouldn't replace do nothing in this case? We know it already has the key we just inserted so there's nothing to 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.
Currently it will panic because replace_key
will unwrap the inner option: https://github.com/rust-lang/hashbrown/blob/4c824c548e85d75f95e0dfcee09c62c5203f9a75/src/map.rs#L5539
I'm currently leaning towards just removing OccupiedEntry::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.
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 have no opinion on removal, but my point was that I believe it'd be correct to just check for is_none and then short circuit/do nothing.
This comment has been minimized.
This comment has been minimized.
I assume the PR title is wrong here? This is adding |
Oops, thanks. Rebase error. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
The job Click to see the possible cause of the failure (guessed by this bot)
|
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
Removing this from my review queue while you're waiting for hashbrown... @rustbot author |
Make HashSet::insert return OccupiedEntry See rust-lang/rust#120077 (comment)
@SUPERCILEX any updates on this? thanks |
@Dylan-DPC the hashbrown PR was merged. @Amanieu What are the next steps? |
@Amanieu Can you release hashbrown 0.15 so this can make progress? |
I'm working on a cleanup of the Entry/EntryRef API in hashbrown. Once that's done I will publish 0.15. |
See https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/HashSet.3A.3Aentry/near/413224639 and #60896 (comment)
Closes rust-lang/rfcs#1490