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

Add support for 128bit numbers #237

Merged
merged 2 commits into from May 30, 2018
Merged

Add support for 128bit numbers #237

merged 2 commits into from May 30, 2018

Conversation

@KodrAus
Copy link
Contributor

KodrAus commented May 30, 2018

Closes #236

This PR adds support for encoding/decoding i128 and u128 which were stabilized in rustc 1.26.0, and supported as of serde 1.0.60.

There's a new a disabled-by-default i128 feature flag in bincode, which enables support for 128bit numbers. If bincode is compiled on a version of Rust that supports 128bit numbers but doesn't have the i128 feature enabled then we'll return a custom error at runtime suggesting they enable the feature.

If there's anything you'd like changed let me know. Thanks!

@KodrAus
Copy link
Contributor Author

KodrAus commented May 30, 2018

@KodrAus
Copy link
Contributor Author

KodrAus commented May 30, 2018

I could rejig the .travis.yml a bit so we just build without the feature, and test with it so there's less output to worry about? Would you be interested in pinning a specific version of Rust in CI so we don't accidentally bump it when making changes? I was testing these changes locally on the latest stable and 1.18.0?

@TyOverby
Copy link
Collaborator

TyOverby commented May 30, 2018

This looks great! Thanks!

My only request would be adding a test that utilizes the full range of the 128 bit numbers.

@TyOverby
Copy link
Collaborator

TyOverby commented May 30, 2018

I'm totally fine with the travis output spew as I rarely need to look at it :D

As for pinning the version, I don't know? It looks like serde tests with

  • 1.13.0
  • 1.15.0
  • stable
  • beta
  • nightly

If you'd like, feel free to build these into this PR (in a new commit) and we can enable testing on these channels.

@KodrAus KodrAus force-pushed the KodrAus:i128 branch from ed68c92 to 4505172 May 30, 2018
@KodrAus
Copy link
Contributor Author

KodrAus commented May 30, 2018

@TyOverby It looks like we need 1.18.0 for the pub(restricted) syntax, so I've added that to the CI.

@TyOverby
Copy link
Collaborator

TyOverby commented May 30, 2018

Looks great, thanks for the contributions!

@TyOverby TyOverby merged commit bca4b65 into servo:master May 30, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.