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

i128 #60

Merged
merged 22 commits into from
May 8, 2018
Merged

i128 #60

merged 22 commits into from
May 8, 2018

Conversation

regexident
Copy link
Contributor

No description provided.

@cuviper
Copy link
Member

cuviper commented Apr 9, 2018

Thanks! This will be nice to have with the upcoming i128 stabilization.

You'll need to declare the feature in Cargo.toml, and test it in ci/test_full.sh for sufficiently new rustc only. Please also document the feature in README.md.

Do you plan on completing the rest of the applicable traits too? I notice at least Num, Unsigned, and cast::* remaining.

@regexident
Copy link
Contributor Author

You'll need to declare the feature in Cargo.toml, and test it in ci/test_full.sh for sufficiently new rustc only. Please also document the feature in README.md.

Done.

Do you plan on completing the rest of the applicable traits too? I notice at least Num, Unsigned, and cast::* remaining.

  • Num
  • Unsigned
  • cast::*

I'd prefer to leave adding i128/u128 to cast.rs to somebody more familiar with the file content's nature, tbh.

@regexident
Copy link
Contributor Author

regexident commented Apr 17, 2018

Any further feedback? What needs to be done to proceed? Do we want to proceed?


The compiler (v1.8.0) also appears to trip over the u128/i128 value suffix:

src/identities.rs:45:19: 45:24 error: invalid width 128 for integer literal
src/identities.rs:45 zero_impl!(u128, 0u128);

Any idea what to do about this? What is num-trait's stance on language backwards compatibility?

@cuviper
Copy link
Member

cuviper commented Apr 17, 2018

What needs to be done to proceed?

I'd like to add the cast stuff -- just need to get around to it.

The compiler (v1.8.0) also appears to trip over the u128/i128 value suffix:

Does it work if you leave it as a bare 0? I think the exact type can be inferred, at least for Zero::zero(). The comparison in is_zero might have type-inference trouble still, but we can probably rewrite that as *self == $t::zero() instead.

What is num-trait's stance on language backwards compatibility?

I take a firm stance on this, which is why it's so old at 1.8 right now.

@regexident
Copy link
Contributor Author

What needs to be done to proceed?

I'd like to add the cast stuff -- just need to get around to it.

Awesome!

The compiler (v1.8.0) also appears to trip over the u128/i128 value suffix:

Does it work if you leave it as a bare 0? I think the exact type can be inferred, at least for Zero::zero(). The comparison in is_zero might have type-inference trouble still, but we can probably rewrite that as *self == $t::zero() instead.

I removed the suffixes (from all invocations of zero_impl!/one_impl! for the sake of consistency).

The code now however (and despite being hidden behind a feature flag) fails to compile with:

error: type name `u128` is undefined or not in scope `[E0412]`

🤔

What is num-trait's stance on language backwards compatibility?

I take a firm stance on this, which is why it's so old at 1.8 right now.

👍

ci/test_full.sh Outdated

# test `i128`
cargo build --verbose --features=i128
cargo test --verbose --features=i128
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be conditioned on TRAVIS_RUST_VERSION, so it's only attempted on Rust 1.26+ (currently just beta and nightly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So remove redundant …

$run cargo build --verbose

… from rustup.sh?

And add …

if [[ "$TRAVIS_RUST_VERSION" =~ ^(nightly|beta|stable)$ ]]; then
    cargo build --verbose --features=i128
    cargo test --verbose --features=i128
fi

… to test_full.sh?

Copy link
Member

Choose a reason for hiding this comment

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

rustup.sh should be harmless, but you can tweak that if you like. test_full.sh is called directly in .travis.yml -- I guess we could remove the redundant build there too. Doesn't really matter.

The condition looks fine, but I doubt stable 1.25 will work yet, as the feature was only recently stabilized for the upcoming 1.26.

@cuviper cuviper mentioned this pull request May 2, 2018
This includes new conditional methods `ToPrimitive::{to_i128,to_u128}`
and `FromPrimitive::{from_i128,from_u128}`.  Since features can only be
additive, these methods must not cause a breaking change to anyone when
enabled -- thus they have a default implementation that converts through
64-bit values.  Types that can do better with a full 128-bit integer,
like bigint or floating-point, will probably want to override these.
@cuviper
Copy link
Member

cuviper commented May 7, 2018

@regexident - I've pushed 128-bit casts, and a few other things. I'd appreciate if you could look over my changes too!

@regexident
Copy link
Contributor Author

@cuviper looks good to me! 💪

@cuviper
Copy link
Member

cuviper commented May 8, 2018

Thanks!

bors r+

bors bot added a commit that referenced this pull request May 8, 2018
60: i128 r=cuviper a=regexident



Co-authored-by: Vincent Esche <regexident@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors
Copy link
Contributor

bors bot commented May 8, 2018

Build succeeded

@bors bors bot merged commit 1af2319 into rust-num:master May 8, 2018
@regexident regexident deleted the i128 branch May 9, 2018 07:11
@regexident
Copy link
Contributor Author

FYI:

docs.rs failed to build num-traits-0.2.3
Please check build logs and if you believe this is docs.rs' fault, report into this issue report.

https://docs.rs/crate/num-traits/0.2.3/builds/99099

@cuviper
Copy link
Member

cuviper commented May 10, 2018

Yeah. 😢 I already reported this in https://github.com/onur/docs.rs/issues/23#issuecomment-388160926

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.

None yet

2 participants