Skip to content

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