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 u128/i128 support #13

Merged
merged 5 commits into from
May 6, 2017
Merged

Implement u128/i128 support #13

merged 5 commits into from
May 6, 2017

Conversation

est31
Copy link
Contributor

@est31 est31 commented May 6, 2017

Fixes #12

Unfortunately only commented out tests, due to BurntSushi/quickcheck#162

@est31
Copy link
Contributor Author

est31 commented May 6, 2017

r? @japaric

Also please don't forget to publish a new version (0.2.1?).

Copy link
Owner

@japaric japaric left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @est31. This looks good to me except for the "lossless" u128 -> f32 conversion. I'd prefer to skip that for now. Would that be good enough for the compiler-builtins PR?

.travis.yml Outdated
@@ -9,13 +9,13 @@ env: TARGET=x86_64-unknown-linux-gnu
matrix:
include:
- env: TARGET=i686-unknown-linux-gnu
- env: TARGET=x86_64-unknown-linux-gnu
- env: TARGET=x86_64-unknown-linux-gnu RUST_BETA=1
Copy link
Owner

Choose a reason for hiding this comment

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

nit: the TRAVIS_RUST_VERSION variable will contain the string 'beta' or 'nightly' when 'rust: beta' or 'rust: nightly' is used here. You can use that var instead of defining a new one.

src/lib.rs Outdated
u32 => i128, u128;
u64 => i128, u128;
usize => i128, u128;
u128 => f32, f64, u128;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if u128 -> f32 can be really be considered a promotion.

f32::MAX prints 340282350000000000000000000000000000000.
u128::MAX prints 340282366920938463463374607431768211455

So u128::MAX is larger than f32::MAX or IOW u128::MAX doesn't fit in f32. IMO, this conversion should return Error::Overflow. However, rustc (and gcc) seem to be fine with the u128::MAX as f32 operation; it returns inf. That seems wrong to me -- that's not loss of precision; that's a rounding operation with a rounding error of inf.

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've modified it to make it pass now.

@est31
Copy link
Contributor Author

est31 commented May 6, 2017

The review should be addressed now.

@japaric
Copy link
Owner

japaric commented May 6, 2017

Thanks @est31.

@homunkulus r+

@homunkulus
Copy link
Collaborator

📌 Commit ca5aaef has been approved by japaric

@homunkulus
Copy link
Collaborator

⌛ Testing commit ca5aaef with merge ca5aaef...

@homunkulus
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: japaric
Pushing ca5aaef to master...

@homunkulus homunkulus merged commit ca5aaef into japaric:master May 6, 2017
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.

3 participants