Skip to content

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

Merged
merged 7 commits into from
Aug 15, 2022

Conversation

ueco-jb
Copy link
Contributor

@ueco-jb ueco-jb commented Jul 11, 2022

closes #1349

@ueco-jb ueco-jb requested a review from webmaster128 July 11, 2022 15:18
@ueco-jb ueco-jb self-assigned this Jul 11, 2022
Copy link
Member

@webmaster128 webmaster128 left a 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
}
}
Copy link
Member

@webmaster128 webmaster128 Jul 11, 2022

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

Copy link
Contributor

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.

Copy link
Contributor

@uint uint Aug 15, 2022

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 {

Copy link
Member

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)?

Copy link
Contributor

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.

@webmaster128 webmaster128 added this to the 1.1.0 milestone Jul 16, 2022
@ueco-jb ueco-jb removed this from the 1.1.0 milestone Jul 31, 2022
@uint
Copy link
Contributor

uint commented Aug 8, 2022

Are you still working on this?

@ueco-jb
Copy link
Contributor Author

ueco-jb commented Aug 8, 2022

Yes.

@uint
Copy link
Contributor

uint commented Aug 15, 2022

Resolved CHANGELOG.md conflict.

@uint
Copy link
Contributor

uint commented Aug 15, 2022

@webmaster128 This last commit should force the type resolution to make those ints u64 from the beginning, rather than possibly casting them.

@webmaster128
Copy link
Member

Nice, thanks!

@uint uint merged commit c106596 into main Aug 15, 2022
@uint uint deleted the 1349-implement-partialeq-for-reference-math-types branch August 15, 2022 12:53
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.

Implement PartianEq for reference for math types
3 participants