-
Notifications
You must be signed in to change notification settings - Fork 160
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 PartialEq and Eq #23
Conversation
src/lib.rs
Outdated
@@ -1463,6 +1463,27 @@ impl<K, V, S> Default for OrderMap<K, V, S> | |||
} | |||
} | |||
|
|||
impl<K, V, S> PartialEq for OrderMap<K, V, S> | |||
where K: Eq + Hash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to do this more generally than libstd.
We sometimes have room to fix their mistakes or oversights, right? And we should do it if it remains a drop in replacement.
What I first thought of here is that PartialEq should not require that both maps have the same hasher. But in fact, we can generalize it so that neither K, V, S need to be the same as long as they implement V1: PartialEq<V2>
and so on. Does that make sense to you?
For K
it's a bit trickier, we need to be able to look up using one key in the other map. I'm not sure what works there without trying it out in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good idea!
For K
, my first thought is std::convert
traits and Borrow
, but later on I found none of them are suitable here. PartialEq
requires symmetric, a == b implies b == a. I'm not sure what is best for K
, maybe they should be identical. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm we can wait a bit with K and merge the Equivalent PR. It's a generalization of the Borrow mechanism #10
I'll have to experiment with the K issue |
Hm we can merge this since the Equivalent trait is waiting on the next version anyway, so we can do a small type inference break then. |
PartialEq should be exercised in quickcheck tests. Ideally you'd just add a new one next to the ones that test equivalence with HashMap. This test can be added by you or I, depending on who gets there first. Edit: I got there first |
Hello again!
This PR implements traits
PartialEq
andEq
. 😀Cc #19