-
-
Notifications
You must be signed in to change notification settings - Fork 777
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 Serde impls for i128 and u128 #1263
Conversation
929ac60
to
080b4a6
Compare
a8fc964
to
c7fce79
Compare
What's the process like to get this merged? Support for i128 and u128 would be awesome. |
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. |
@oli-obk can you notice any problems with this approach? |
There was a problem hiding this 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?
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. |
Fixes #1136.