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

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 <alishergaliev88@gmail.com>
cuviper added a commit to cuviper/hashbrown that referenced this pull request Sep 18, 2024
Co-authored-by: JustForFun88 <alishergaliev88@gmail.com>
cuviper added a commit to cuviper/hashbrown that referenced this pull request Sep 18, 2024
Co-authored-by: JustForFun88 <alishergaliev88@gmail.com>
# 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.

Violating the set invariant with HashSet::get_or_insert_with
3 participants