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

Partially fix Decimal.ulp to return a valid representation. #3014

Merged
merged 1 commit into from
Jul 25, 2021

Conversation

xwu
Copy link
Contributor

@xwu xwu commented Jul 25, 2021

Unfortunately, Decimal.ulp has never actually worked correctly. To make it short, users currently get an invalid representation of an incorrect result

  1. The incorrect result: The implementation has never actually returned the "unit in the last place" as an IEEE floating-point type would. For instance, print((0.1 as Decimal).ulp) gives 0.1, but it's obvious that the type has enough precision to be able to represent values between 0.1 and 0.2. This is somewhat forgivable, though, because Foundation.Decimal isn't an IEEE decimal type.

  2. The invalid representation: The implementation constructs a Decimal value explicitly with _length set to 8 but using a mantissa of only length 1. This breaks Foundation.Decimal invariants and is not normalized by NSDecimalNormalize. Consequently, all sorts of weirdness happens when one tries to compare this invalid representation to valid ones. For instance, (0.1 as Decimal).ulp == 0.1 evaluates to false, which is untenable because, as we just noted above, print((0.1 as Decimal).ulp) gives 0.1.

This PR defers addressing (1), since it should be done in concert with addressing the implementation of nextUp and nextDown, and we'd need to consider whether that's a straightforward bugfix or whether it would break too much existing code that relies on the current behavior of these properties. What this PR does do is address (2) by changing the explicitly set _length to 1; corresponding tests are added and adjusted as appropriate.

@xwu xwu force-pushed the decimal-ulp-length branch from 1de8de0 to 8a020be Compare July 25, 2021 03:39
@xwu
Copy link
Contributor Author

xwu commented Jul 25, 2021

@swift-ci test

@xwu xwu requested a review from spevans July 25, 2021 03:40
@xwu xwu merged commit 774d396 into swiftlang:main Jul 25, 2021
@xwu xwu deleted the decimal-ulp-length branch July 25, 2021 20:09
@xwu
Copy link
Contributor Author

xwu commented Jul 25, 2021

@spevans Should this be cherrypicked for the Swift 5.5 branch (or even Swift 5.4)?

@spevans
Copy link
Contributor

spevans commented Jul 26, 2021

@xwu Yes, I think this is appropriate for both 5.5 and 5.4, thanks

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