-
Notifications
You must be signed in to change notification settings - Fork 375
Implement PartialEq
for references on all math types
#1350
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
Conversation
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.
LGTM. Can consistency be enforced with
+ PartialEq
+ PartialEq<&'a Self>
in the AllImpl
trait?
fn eq(&self, rhs: &&Decimal) -> bool { | ||
self == *rhs | ||
} | ||
} |
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.
Ah, can we implement that using forward_ref_binop!
too? That should give us 3 implementations: reference left, right and both
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.
Doesn't seem so. forward_ref_binop!
looks for an Output
associated type (like when you're trying to do this for Add
), but PartialEq
doesn't have such a type.
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.
We also don't need an implementation for refs on both sides. That case is taken care of by deref coercion.
We're only missing one for LHS, like:
impl PartialEq<Decimal> for &Decimal {
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.
Ah, good to know, thanks @uint. Can you add tests for all combinations and the missing implementation (maybe as PR into this PR)?
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.
Sure thing. Let's get 1.1.0
wrapped up.
Are you still working on this? |
Yes. |
Implement PartialEq for references on all math types - LHS and tests
Resolved |
@webmaster128 This last commit should force the type resolution to make those ints |
Nice, thanks! |
closes #1349