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

Implement << and <<= for all uints #1723

Merged
merged 6 commits into from
Jun 15, 2023
Merged

Implement << and <<= for all uints #1723

merged 6 commits into from
Jun 15, 2023

Conversation

chipshort
Copy link
Collaborator

@chipshort chipshort commented Jun 14, 2023

closes #1712

@chipshort chipshort marked this pull request as ready for review June 15, 2023 11:31
@chipshort chipshort requested a review from webmaster128 June 15, 2023 12:04
impl IntImpl<'_> for Uint64 {}
impl IntImpl<'_> for Uint128 {}
impl IntImpl<'_> for Uint256 {}
impl IntImpl<'_> for Uint512 {}
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove impl AllImpl<'_> for Uint512 {} and friends now? They should be required implicitly through the supertrait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only if we copy all the bounds from AllImpl to IntImpl. The way it is right now, IntImpl requires that they have AllImpl too.

Copy link
Member

Choose a reason for hiding this comment

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

I'm referring to line 67-70 in the new code. There we implement

    impl AllImpl<'_> for Uint64 {}
    impl AllImpl<'_> for Uint128 {}
    impl AllImpl<'_> for Uint256 {}
    impl AllImpl<'_> for Uint512 {}

which requires the Uint* types to implement the AllImpl traits. However, line 71-74 should imply that.

E.g. impl IntImpl<'_> for Uint64 {} failes to compile if Uint64 misses Add<&'a Self> since IntImpl requires AllImpl which requires Add<&'a Self>.

Or am I missing something here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know what you mean, but that's not how it works. Let's use Uint64 as an example: Line 71 implies that it implements AllImpl and all the Sh* traits. If we remove line 67, then it does not implement AllImpl anymore and line 71 will fail to compile. AllImpl does not get automatically implemented just because all the bounds are implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Right you are, amen

packages/std/src/math/uint128.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint128.rs Outdated Show resolved Hide resolved
@webmaster128
Copy link
Member

Ah, some CHANGELOG entry would be nice

@chipshort chipshort merged commit 3333590 into 1.2 Jun 15, 2023
@chipshort chipshort deleted the 1712-left-shifts branch June 15, 2023 15:04
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.

2 participants