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

Implement with_hasher adaptors #1007

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

424ever
Copy link

@424ever 424ever commented Dec 23, 2024

Closes #998

Implementations for the following methods:

  • duplicates_with_hasher
  • duplicates_by_with_hasher
  • unique_with_hasher
  • unique_by_with_hasher
  • all_unique_with_hasher
  • into_group_map_with_hasher
  • into_group_map_by_with_hasher
  • into_grouping_map_with_hasher
  • into_grouping_map_by_with_hasher
  • counts_with_hasher
  • counts_by_with_hasher

Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nit, but otherwise this LGTM.

src/duplicates_impl.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@phimuemue
Copy link
Member

Two nits:

  • I'm a bit unhappy about duplicated tests. We can leave them as is and add a TODO that we'd ideally want to write all test cases once and apply them to both normal and _with_hasher functions automagically.

  • (Possibly over-nitpicky:) Should we rely on the fact that RandomState is the default BuildHasher or should we extract it so that std can do whatever it wants and we just adapt it? Could be done as follows:

    trait THashMap {
        type BuildHasher;
    }
    impl<K, V, S> THashMap for std::collections::HashMap<K, V, S> {
        type BuildHasher = S;
    }
    type StdDefaultBuildHasher = <std::collections::HashMap<(), ()> as THashMap>::BuildHasher;
    
    

@jswrenn Feel free to proceed as you deem appropriate.

Copy link

codecov bot commented Jan 4, 2025

Codecov Report

Attention: Patch coverage is 99.55157% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.67%. Comparing base (6814180) to head (625f85f).
Report is 134 commits behind head on master.

Files with missing lines Patch % Lines
src/grouping_map.rs 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
+ Coverage   94.38%   94.67%   +0.28%     
==========================================
  Files          48       50       +2     
  Lines        6665     7062     +397     
==========================================
+ Hits         6291     6686     +395     
- Misses        374      376       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jswrenn
Copy link
Member

jswrenn commented Jan 15, 2025

  • Should we rely on the fact that RandomState is the default BuildHasher or should we extract it so that std can do whatever it wants and we just adapt it?

It would be a breaking change for Rust to change its default hasher, so we're fine to rely on RandomState directly.

Comment on lines +29 to +46
// A Hasher which forwards it's calls to RandomState to make sure different hashers
// are accepted in the various *_with_hasher methods.
#[derive(Default)]
struct TestHasher(RandomState);

impl TestHasher {
fn new() -> Self {
TestHasher(RandomState::new())
}
}

impl BuildHasher for TestHasher {
type Hasher = <RandomState as BuildHasher>::Hasher;

fn build_hasher(&self) -> Self::Hasher {
self.0.build_hasher()
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this buy us much? If it behaves identically to RandomState, then what's the advantage of newtyping it? I'm not worried that we've defined our with_hasher methods to literally only accept RandomState.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because RandomState is the default for the type parameters, it's possible to implement the types only for S = RandomState. This is just the easiest way I thought of to catch that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. We can test that more directly by doing:

let _: HashMap<_, _, TestHasher> = empty::<u8>().into_group_map_with_hasher(TestHasher::new());

No need to then repeat the assertions on the output value.

New calls to the *_with_hasher methods are added to make sure they are callable with non-default hashers and provide the correct types.
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.

Provide with_hasher alternative adaptors
3 participants