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.

Sign up for GitHub

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

Jump to bottom

Add Serde impls for i128 and u128 #1263

Merged
merged 8 commits into from May 25, 2018
Merged

Add Serde impls for i128 and u128 #1263

merged 8 commits into from May 25, 2018

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented May 18, 2018

Fixes #1136.

@dtolnay dtolnay added the wip label May 18, 2018
@dtolnay dtolnay force-pushed the integer128 branch 5 times, most recently from 929ac60 to 080b4a6 Compare May 19, 2018 22:46
@dtolnay dtolnay mentioned this pull request May 19, 2018
Closed
@dtolnay dtolnay removed the wip label May 19, 2018
@dtolnay dtolnay force-pushed the integer128 branch 3 times, most recently from a8fc964 to c7fce79 Compare May 20, 2018 22:48
@cgm616
Copy link

cgm616 commented May 21, 2018

What's the process like to get this merged? Support for i128 and u128 would be awesome.

@dtolnay
Copy link
Member Author

dtolnay commented May 21, 2018

I am planning to land this on 25 May to give people some time to complain about our new build script. It becomes complicated to reverse course on the build script after starting to stick observable behavior behind it.

@dtolnay dtolnay requested a review from oli-obk May 24, 2018 17:13
@dtolnay
Copy link
Member Author

dtolnay commented May 24, 2018

@oli-obk can you notice any problems with this approach?

oli-obk
oli-obk approved these changes May 25, 2018
View reviewed changes
Copy link
Member

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

That should work exactly as advertised! I don't see anything wrong with this approach.

Do we have a travis builder actually testing it?

@dtolnay
Copy link
Member Author

dtolnay commented May 25, 2018

Thanks, I really appreciate the second opinion. In terms of CI -- our existing 1.13.0 builder confirms that things continue to compile on a compiler without these types. Our nightly builder that runs the test suite is running test_de::test_small_int_to_128 and test_ser::test_integer128. The usefulness of those test cases is limited by not having 128-bit tokens in serde_test (#1281) but at least they confirm that deserialize_?128 and serialize_?128 are invoked as intended. Upon implementing #1280 at least there will be a Deserializer directly in the Serde codebase that deals with 128-bit integers that can be tested. And serde_json will provide end-to-end test coverage when we add 128-bit support there.

@dtolnay dtolnay merged commit b279ebb into master May 25, 2018
@dtolnay dtolnay deleted the integer128 branch May 25, 2018 23:03
@hcpl hcpl mentioned this pull request May 29, 2018
Closed
@hcpl hcpl mentioned this pull request Sep 1, 2018
Merged
@segeljakt segeljakt mentioned this pull request Apr 29, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

i128 and u128 integers missing Deserialize impls
3 participants
@dtolnay @cgm616 @oli-obk