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 FixedDecimal rounding bug; add more tests #3644

Merged
merged 7 commits into from
Jul 7, 2023
Merged

Conversation

sffc
Copy link
Member

@sffc sffc commented Jul 6, 2023

Numbers with interior zeros were expanding at too high a magnitude. For example, consider the input 1.108 expanded at position -2.

Old behavior:

  1. Remove the digits beyond position -2 to get 1.10 (digits list 110)
  2. Normalize the internal representation (digits list 11)
  3. Increment the last digit to get 1.20

New behavior:

  1. Remove the digits beyond position -2 to get 1.10 (digits list 110)
  2. Increment the last digit to get 1.11
  3. Assert that the internal representation is normalized

I had the individual failing input from @Manishearth, but I had to write some elaborate test cases before I could reproduce the problem from "first principles". Since I was able to do that, I'm confident that this change fixes the problem across all rounding modes.

@sffc sffc requested review from younies and Manishearth July 6, 2023 22:18
Manishearth
Manishearth previously approved these changes Jul 6, 2023
@Manishearth
Copy link
Member

cc @eggrobin who was also observing interestingly as I was poking at this failure

@sffc
Copy link
Member Author

sffc commented Jul 7, 2023

I went ahead and refactored the logic of the trunc and expand functions to be more clear and consistent and avoid calling into other helper functions that obscured the behavior.

Manishearth
Manishearth previously approved these changes Jul 7, 2023
younies
younies previously approved these changes Jul 7, 2023
@sffc sffc dismissed stale reviews from younies and Manishearth via c3dd66c July 7, 2023 12:22
@sffc sffc requested a review from younies July 7, 2023 13:43
@sffc sffc merged commit d1cc1a6 into unicode-org:main Jul 7, 2023
22 checks passed
@sffc sffc deleted the fd-fix branch July 7, 2023 13:44
@Manishearth Manishearth mentioned this pull request Sep 21, 2023
13 tasks
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.

3 participants