Skip to content

Conversation

@mjclarke94
Copy link
Contributor

Attempts to close #32. Still needs documentation etc, but figured I'd get the initial PR up for code review.

I'm fairly new to rust so feel free to be brutal here! It's likely that I'm missing something obvious here.

I was looking into whether it would be plausible to also be generic over the map type (say, to allow BTreeMap instead of HashMap which might be handy when it comes to things like k_most_common, but was struggling to see how to make it be generic over both.

@mjclarke94 mjclarke94 marked this pull request as ready for review December 3, 2024 17:41
@coriolinus
Copy link
Owner

Thanks for the contribution! It's likely to be a few days before I can properly review this--life is hectic right now--but I have seen it, and it's on my task queue.

@mjclarke94
Copy link
Contributor Author

I'm installing off my fork for the time being so no rush on my end :)
This is just the result of me getting carried away trying to optimise an advent of code solution!

@coriolinus
Copy link
Owner

Good news! I'm finally back and able to review this, after "a few days" somehow turned into half a year.

Bad news: I made some changes, not to this pr. I think they'll make the PR cleaner, but it means there are now some merge conflicts.

Can't expect better behavior from anyone else than I can of myself. So! Let's get this fixed up and merged by 2026!

Adds a third generic parameter S (defaulting to RandomState) to Counter,
allowing users to specify custom hash builders for the underlying HashMap.
This enables use cases like deterministic hashing or performance optimizations.

Key changes:
- Add generic parameter S = RandomState to Counter struct
- Update all trait implementations to include the S parameter
- Add BuildHasher bounds only where HashMap operations require them
- Add Default bounds only for constructors
- Include test demonstrating custom hasher usage

Closes coriolinus#32
@mjclarke94
Copy link
Contributor Author

I'll stretch for winter 2025 just so it's ready for the next advent of code!

Should merge cleanly now

Copy link
Owner

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Thanks for the patience, and for this work!

@coriolinus coriolinus merged commit 07e6344 into coriolinus:master Aug 13, 2025
1 check passed
@govinda-kamath
Copy link

Looks like this broke the build with serde enabled. Fixed here: #54

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.

Suggestion: make Counter generic over the hashing algorithm

3 participants