Skip to content

[JSONSerialization] Always use reference numeric types to match Darwin behavior #1634

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

Merged
merged 1 commit into from
Jul 16, 2018

Conversation

xwu
Copy link
Contributor

@xwu xwu commented Jul 13, 2018

This PR removes the undocumented internal serialization option useReferenceNumericTypes and aligns behavior with Darwin Foundation. (Note: there remains a divergence between platforms when it comes to 128-bit integers.)

The parsing logic is simplified and tests are expanded to ensure that "[10]" is deserialized to a value that can be cast to Int or Double.

@xwu
Copy link
Contributor Author

xwu commented Jul 13, 2018

/cc @johnno1962

@xwu
Copy link
Contributor Author

xwu commented Jul 13, 2018

@swift-ci please test

@xwu
Copy link
Contributor Author

xwu commented Jul 13, 2018

@johnno1962 Whoops, looks like we did the same thing. Sorry!

@johnno1962
Copy link
Contributor

johnno1962 commented Jul 13, 2018

Pinch my PR will you 😀!. Yours is better, run with it. Don’t forget to cherry pick into swift-4.2-branch

@xwu xwu changed the title [JSONSerialization] Always use reference types to match Darwin behavior [JSONSerialization] Always use reference numeric types to match Darwin behavior Jul 13, 2018
@xwu xwu requested review from millenomi and parkera July 13, 2018 03:26
@parkera parkera requested a review from itaiferber July 14, 2018 19:51
@parkera
Copy link
Contributor

parkera commented Jul 14, 2018

Looks good to me. I'm assuming we can do this now because of the bridging improvements?

@johnno1962
Copy link
Contributor

Yes, this is following through on #1443

Copy link
Contributor

@itaiferber itaiferber left a comment

Choose a reason for hiding this comment

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

The changes here specifically LGTM. I wonder if now that we can finally make use of bridging whether we should move off Int as the primary integer type to Int64 and add support for UInt64 to get the full range of integer values as we do on Darwin.

@xwu
Copy link
Contributor Author

xwu commented Jul 16, 2018

@itaiferber Certainly a good idea; I'll keep this PR limited to the changes here, and that can be a follow-on improvement. As I mention, we still have some discrepancies in that department with respect to Darwin support for 128-bit integers.

@itaiferber
Copy link
Contributor

@swift-ci Please test and merge

@swift-ci swift-ci merged commit 8b7a39b into swiftlang:master Jul 16, 2018
@itaiferber
Copy link
Contributor

@xwu Now that this is merged, please also cherry pick into swift-4.2-branch

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.

5 participants