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

Fix two Decimal members with respect to NaN #2377

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

xwu
Copy link
Contributor

@xwu xwu commented Jun 24, 2019

This PR corresponds to swiftlang/swift#25698.

It fixes Decimal.magnitude so that Decimal.nan.magnitude is no longer incorrectly 0.

The corresponding PR in the apple/swift repository also incorporates changes from #1759; here, minor edits (NFC), including one suggested on review, are back-propagated so as to help the two versions of Foundation.Decimal remain in sync.

Resolves SR-10938.

@xwu xwu requested a review from spevans June 24, 2019 22:28
@xwu
Copy link
Contributor Author

xwu commented Jun 24, 2019

@swift-ci Please test

@xwu
Copy link
Contributor Author

xwu commented Jun 24, 2019

@spevans There's at least one other fix that isn't synchronized between here and the overlay. One thing I'd like to do (or rather, have already done but would like to contribute in a PR) is to reorganize Decimal.swift and bring the two implementations back into alignment where it makes sense to do so. Even the organization of two files has fallen out of sync, making it increasingly difficult to ensure parity. Does that sound sensible to you?

@spevans
Copy link
Contributor

spevans commented Jun 24, 2019

This looks fine but might be worth adding a few tests for .magnitude of various numbers, eg the Decimal constants Decimal.leastFiniteMagnitude, Decimal.greatestFiniteMagnitude etc and maybe Decimal(1), Decimal(-1)

@xwu

This comment has been minimized.

1 similar comment
@xwu

This comment has been minimized.

@spevans
Copy link
Contributor

spevans commented Jun 25, 2019

Yes, definitely a good idea to resync the two versions. I think the scl-f version may be slightly ahead in bug fixes but there should be enough tests in it to cover any slight regressions. Probably worth doing a couple of PRs to resync them.

Copy link
Contributor

@spevans spevans left a comment

Choose a reason for hiding this comment

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

Could you just squash these down into one commit and then it can be merged, thanks

@xwu xwu force-pushed the magnitude-of-nan branch from 407ec47 to b7e9f7f Compare June 25, 2019 12:40
@xwu

This comment has been minimized.

1 similar comment
@xwu
Copy link
Contributor Author

xwu commented Jun 25, 2019

@swift-ci Please test Linux

@xwu xwu merged commit d5d2187 into swiftlang:master Jun 25, 2019
@xwu xwu deleted the magnitude-of-nan branch June 25, 2019 14:26
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.

2 participants