Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add an Entry API for HashSet #342

Merged
merged 1 commit into from
Jun 18, 2022
Merged

Add an Entry API for HashSet #342

merged 1 commit into from
Jun 18, 2022

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jun 16, 2022

This adds HashSet::entry and rustc_entry, mimicking the API of HashMap. The "rustc" variants are without the S type parameter, just like the difference on maps. Set entries have fewer methods since there's no mutable value like in maps, but the basic insert/remove/replace functionality is there.

Resolves #274.

@Amanieu
Copy link
Member

Amanieu commented Jun 17, 2022

Could we avoid introducing another rustc_entry? This was a hack for compatibility with std which didn't have a S parameter on the Entry type. I would prefer if we did this "correctly" and had a S parameter for the version used by std.

@cuviper
Copy link
Member Author

cuviper commented Jun 17, 2022

I expect that std::collections::hash_set::Entry<T> should look like a simple wrapper on hash_map::Entry<T, ()>, even if that's not really direct because it goes through hashbrown. I think it would be unfortunate if std were inconsistent about whether these Entry types have an S parameter, but that's ultimately a libs-api decision.

If you still want to push for that S, then I'll drop rustc_entry here and keep it in a branch on the side, just in case. :)

@Amanieu
Copy link
Member

Amanieu commented Jun 17, 2022

I'll repeat that Entry not having S was a mistake that we shouldn't carry over to new APIs. The rustc_entry in the hashbrown crate is actually slightly less efficient because it has to reserve space for a value ahead of time before knowing whether a value will be inserted into the map. It is better to defer the reservation until a value is explicitly inserted, which requires the S parameter.

@cuviper
Copy link
Member Author

cuviper commented Jun 17, 2022

OK, I've dropped the S-less one.

@Amanieu
Copy link
Member

Amanieu commented Jun 18, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jun 18, 2022

📌 Commit af67f7d has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Jun 18, 2022

⌛ Testing commit af67f7d with merge e54c160...

@bors
Copy link
Contributor

bors commented Jun 18, 2022

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing e54c160 to master...

@bors bors merged commit e54c160 into rust-lang:master Jun 18, 2022
@SUPERCILEX
Copy link
Contributor

Any idea when this will be released?

@Amanieu
Copy link
Member

Amanieu commented Jul 8, 2022

I just published a release.

@SUPERCILEX
Copy link
Contributor

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entry API for HashSet
4 participants