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

Relax bounds on many methods to be minimal #64

Merged
merged 2 commits into from
Feb 26, 2020
Merged

Relax bounds on many methods to be minimal #64

merged 2 commits into from
Feb 26, 2020

Conversation

jonhoo
Copy link
Owner

@jonhoo jonhoo commented Feb 26, 2020

This patch removes a large number of unnecessary bounds. Beyond the
obvious ones (like iterators do not require Hash and constructors do
not need BuildHasher), the biggest change is that the thread-safety
bounds are now only placed on methods that modify the map. The
reasoning here is simple enough: if the threads-safety bounds do not
hold for K/V, then the map must be empty, and if the map is empty, the
read operations are fine even if the K/V type is not thread-safe.

Inspired by rust-lang/rust#67642


This change is Reviewable

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 26, 2020

Sorry for all the code motion — this also groups together methods that have the same bounds.
The bodies of the methods have not changed.

@codecov
Copy link

codecov bot commented Feb 26, 2020

Codecov Report

Merging #64 into master will increase coverage by 1.57%.
The diff coverage is n/a.

Impacted Files Coverage Δ
src/raw/mod.rs 86.73% <0.00%> (+2.04%) ⬆️
src/map.rs 89.29% <0.00%> (+2.19%) ⬆️

This patch removes a large number of unnecessary bounds. Beyond the
obvious ones (like iterators do not require `Hash` and constructors do
not need `BuildHasher`), the biggest change is that the thread-safety
bounds are now _only_ placed on methods that modify the map. The
reasoning here is simple enough: if the threads-safety bounds do not
hold for K/V, then the map must be empty, and if the map is empty, the
read operations are fine even if the K/V type is not thread-safe.
src/map.rs Outdated Show resolved Hide resolved
@domenicquirl
Copy link
Collaborator

What a confusing diff... apart from the review comment I am curious about with_capacity and with_capacity_and_hasher. As is these also require K: Clone, which seems to be due to calling try_presize, which in turn calls transfer in case the table is already initialized and needs to grow.

This is definitely also correct if we initialize from insert or similar, but here we are constructing the map. It feels weird to me that this would require Clone on keys even if no keys are handled. And since we're constructing the map, no one else can have access as we cannot yet have shared it. Would it make sense to add a simplified presize method for this case, which knows it doesn't have to handle transfers and will always just initialize the table?

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 26, 2020

@domenicquirl yeah, I had the same though going through it. As to whether it makes sense to special-case, I'm genuinely not sure. Maybe? My inclination would be to land this first though, and then add a specialized variant of presize that knows it doesn't have to call transfer. What do you think?

@domenicquirl
Copy link
Collaborator

I think I dislike this enough logically that I am in favour of adding a special case for it. Especially since most of what this change does is allowing for "unused" maps to be constructed without any constraints, in the sense that if one wants to handle entries, then the full trait bounds apply anyways (not that I would have a concrete use case for this). In my opinion it's worth the extra implementation, only we should take care to document this well and make it clear why there are two presize methods.

As for whether to add this now or in a later PR, I don't really care. I will probably touch a few of the trait bounds again anyways for everything that requires Ord + PartialOrd to use tree bins.

@domenicquirl domenicquirl self-requested a review February 26, 2020 23:06
@jonhoo jonhoo merged commit d72da26 into master Feb 26, 2020
@jonhoo jonhoo deleted the relax-bounds branch February 26, 2020 23:11
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.

2 participants