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

Expose U128, add u128 conversions. #92

Merged
merged 4 commits into from
Jan 15, 2019
Merged

Expose U128, add u128 conversions. #92

merged 4 commits into from
Jan 15, 2019

Conversation

tomusdrw
Copy link
Contributor

I think long term it would be good to use u128 as words instead of u64

uint/src/uint.rs Outdated
@@ -414,7 +464,7 @@ macro_rules! construct_uint {
pub fn as_u32(&self) -> u32 {
let &$name(ref arr) = self;
if (arr[0] & (0xffffffffu64 << 32)) != 0 {
panic!("Integer overflow when casting U256")
panic!("Integer overflow when casting to u64")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
panic!("Integer overflow when casting to u64")
panic!("Integer overflow when casting to u32")


impl $crate::core_::convert::From<i128> for $name {
fn from(value: i128) -> $name {
match value >= 0 {
Copy link
Member

@niklasad1 niklasad1 Jan 11, 2019

Choose a reason for hiding this comment

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

this could be match (value as u128) >> 127 == 0 { ... } but the assembly is identical according to https://rust.godbolt.org/z/5kXf8K, so better keep it way more readable

uint/src/uint.rs Outdated
#[inline]
pub fn as_u128(&self) -> u128 {
let &$name(ref arr) = self;
for i in (2..$n_words) {
Copy link
Member

Choose a reason for hiding this comment

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

needless paratheses

Suggested change
for i in (2..$n_words) {
for i in 2..$n_words {

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good, a couple of minor things to clean up.

However, I'm unsure of the differences between uint in this repo and the primitives repo

Would be good add some benchmarks just to see the difference between u64 and u128 (I suspect it's a bit slower but not much)

uint/src/uint.rs Outdated
@@ -841,7 +890,7 @@ macro_rules! construct_uint {
impl_map_from!($name, i32, i64);
impl_map_from!($name, isize, i64);

// Converts from big endian representation of U256
// Converts from big endian representation of $name
Copy link
Member

Choose a reason for hiding this comment

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

This is a comment. Does macro name resolve for comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, seems it doesn't rust-lang/rust#37903

@sorpaas
Copy link
Member

sorpaas commented Jan 12, 2019

@niklasad1 I think we moved uint from primitives repo to here, and that one is removed.

@sorpaas
Copy link
Member

sorpaas commented Jan 12, 2019

Looks like test fails because metadata macro is broken somehow: https://travis-ci.org/paritytech/parity-common/jobs/478340732#L807

@niklasad1
Copy link
Member

@sorpaas

All right I see, makes sense with your pending PR in the `primitives repo´.

@sorpaas sorpaas merged commit 5b56003 into master Jan 15, 2019
@sorpaas sorpaas deleted the td-u128 branch January 15, 2019 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants