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

Const conversions Uint128 -> Uint256 and Uint256 -> Uint512 #1110

Merged
merged 8 commits into from
Sep 27, 2021

Conversation

uint
Copy link
Contributor

@uint uint commented Sep 27, 2021

Closes #1107

@uint
Copy link
Contributor Author

uint commented Sep 27, 2021

Looking commit by commit might make this a bit clearer in this case, I think.

...except for the part where I forgot to update the commit msg. Ahem.

Comment on lines 105 to 116
let words: [u64; 4] = [
u64::from_le_bytes([
bytes[0], bytes[1], bytes[2], bytes[3], bytes[4], bytes[5], bytes[6], bytes[7],
]),
u64::from_le_bytes([
bytes[8], bytes[9], bytes[10], bytes[11], bytes[12], bytes[13], bytes[14],
bytes[15],
]),
0,
0,
];
Self(U256(words))
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be nice to reuse Self::from_be_bytes here? This reduces the places where we mess with U256 internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good idea

packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
0,
];

Self(U512(words))
Copy link
Member

Choose a reason for hiding this comment

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

And with #1111 we can do the same here too, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if that would introduce some overhead though when this function isn't used in a const context. Does it matter? Probably not.

Copy link
Member

Choose a reason for hiding this comment

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

Good question. Compared to every serialization/deserialization all this conversion is trivial. I think it would be good to avoid exposing implementation details here. Ever Uint* type should have efficient BE/LE exporters and importers. Let's focus on those.

bytes[8], bytes[9], bytes[10], bytes[11], bytes[12], bytes[13], bytes[14], bytes[15],
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
])
}
Copy link
Member

Choose a reason for hiding this comment

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

Much nicer!

packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
@uint
Copy link
Contributor Author

uint commented Sep 27, 2021

Rebased.

@webmaster128
Copy link
Member

Do you implement Uint512::from_uint256 via Self::from_le_bytes and add a CHANGELOG entry? Then good to merge.

@uint
Copy link
Contributor Author

uint commented Sep 27, 2021

Do you implement Uint512::from_uint256 via Self::from_le_bytes and add a CHANGELOG entry? Then good to merge.

That's exactly what I was doing 👍

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.

🏅

@uint uint merged commit 06eab90 into main Sep 27, 2021
@uint uint deleted the const-conversions branch September 27, 2021 21:26
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.

Add a const fn converter from Uint128 to Uint256
2 participants