Skip to content
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

Fix HashSet::get_or_insert_with #400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JustForFun88
Copy link
Contributor

Fix #399. Tried to do it without additional overhead. Unless there are additional comparisons.

@JustForFun88 JustForFun88 force-pushed the hashset_get_or_insert_with branch 2 times, most recently from 836fbed to 7f88093 Compare February 16, 2023 13:30
@yyny
Copy link

yyny commented Feb 16, 2023

Should an unsafe _unchecked function also be added? Hashing and comparing the insert_value should be unnecessary if this function is used properly.

@JustForFun88
Copy link
Contributor Author

Should an unsafe _unchecked function also be added? Hashing and comparing the insert_value should be unnecessary if this function is used properly.

Well, strictly speaking, rehashing was present in the old version of the code. Only two comparisons and a panic were added here. Let's see what @Amanieu says :-).

In theory, as I understand it, there is nothing that violates memory or causes UB in the fact that there are two or more identical elements in a Hashmap or HashSet. It just increases the collision, the elements after the first one will be lost and will only show up when iterating. The documentation for HashMap::insert_unique_unchecked scares people a little. Anyway, I can't even imagine when insert_unique_unchecked «operation may panic, loop forever, or any following operation with the map may panic, loop forever or return arbitrary result».

@bors
Copy link
Contributor

bors commented Mar 24, 2023

☔ The latest upstream changes (presumably #390) made this pull request unmergeable. Please resolve the merge conflicts.

@JustForFun88 JustForFun88 force-pushed the hashset_get_or_insert_with branch from db83742 to 63c2dc5 Compare April 9, 2024 17:49
@bors
Copy link
Contributor

bors commented Jun 19, 2024

☔ The latest upstream changes (presumably #533) made this pull request unmergeable. Please resolve the merge conflicts.

cuviper added a commit to cuviper/hashbrown that referenced this pull request Sep 12, 2024
Co-authored-by: JustForFun88 <[email protected]>
cuviper added a commit to cuviper/hashbrown that referenced this pull request Sep 18, 2024
Co-authored-by: JustForFun88 <[email protected]>
cuviper added a commit to cuviper/hashbrown that referenced this pull request Sep 18, 2024
Co-authored-by: JustForFun88 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Violating the set invariant with HashSet::get_or_insert_with
3 participants