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

Add Equivalence trait #45

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Jan 10, 2025

This PR adds get_by_hash() methods that allow for faster lookups when instantiating keys is expensive.

I figured I could get away without writing tests, since the implementation of get() now uses this method internally, so it's implicitly covered :)

Copy link
Owner

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

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

Left a couple of thoughts. If we do end up adding this method, we should probably also add a hasher accessor method to HashMap.

src/map.rs Outdated
/// satisfies the equality function passed.
///
/// This function is similar to [`Self::get()`], but provides a shortcut
/// for lookups where instantiation of keys is expensive. For instance, if
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't this use case be better served by the Equivalent trait? get_by_hash is useful but I think serves a different use case, such as supporting unhashable types or precomputed hashes. For that I wonder if it would be better to expose a lower-level type such as hashbrown's HashTable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point! I've switched to using an Equivalent trait instead, so we don't need an additional method. This also leaves fewer opportunities for consumers to misuse the API.

src/map.rs Outdated Show resolved Hide resolved
@arendjr arendjr changed the title Add get_by_hash() methods Add Equivalence trait Jan 12, 2025
src/raw/mod.rs Outdated
/// ## Contract
///
/// The implementor must hash like K, if it is hashable.
pub trait Equivalent<K>
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine to just pull in the equivalent crate for this instead.

@ibraheemdev
Copy link
Owner

Just doing a ctrl+f for uses of Borrow, I think HashMap(Ref)::{remove, remove_entry} and HashSet(Ref)::remove could use this as well?

@arendjr
Copy link
Contributor Author

arendjr commented Jan 14, 2025

Thanks again for the review, both are done!

@ibraheemdev
Copy link
Owner

Perfect, thanks!

@ibraheemdev ibraheemdev merged commit 0abe12d into ibraheemdev:master Jan 15, 2025
15 checks passed
@ibraheemdev ibraheemdev added the feature New feature or request label Jan 15, 2025
@ibraheemdev
Copy link
Owner

Released in 0.1.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants