Skip to content

Expose U128, add u128 conversions.#92

Merged
sorpaas merged 4 commits intomasterfrom
td-u128
Jan 15, 2019
Merged

Expose U128, add u128 conversions.#92
sorpaas merged 4 commits intomasterfrom
td-u128

Conversation

@tomusdrw
Copy link
Copy Markdown
Contributor

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

Comment thread uint/src/uint.rs Outdated
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
Copy Markdown
Contributor

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

Comment thread uint/src/uint.rs

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

@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

Comment thread 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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

Comment thread uint/src/uint.rs Outdated
impl_map_from!($name, isize, i64);

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

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
Copy Markdown
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
Copy Markdown
Contributor

sorpaas commented Jan 12, 2019

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

@sorpaas
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

@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