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 unexpected Decimal sign behavior from Decimal(sign:exponent:significand:) #834

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EthanHumphrey
Copy link

Resolves #833. Fixes an issue that caused the initialized Decimal's sign not to match the passed sign when significand is negative. More information in the linked issue.

@xwu
Copy link

xwu commented Sep 11, 2024

This would break clients who expect the documented behavior (including any Linux clients that have been able to use the fixed API for basically the whole time) rather than the buggy one. Importantly, there are synchronized changes that would also cause some clients expecting the buggy behavior to break.

For the purposes of preserving existing clients—important, I agree, given that my original bugfix took something like 4 years to be pulled into proprietary Foundation and originally pre-dated ABI stability—both this API and significand should have an ABI-stable entrypoint with the original behavior and the corrected behavior should be decorated with custom mangling.

However, I don't think a fix that changes the type to behave partly one way and partly another is it, as it is liable to create new and interesting incorrect behavior.

@pappalar
Copy link

Agreed the fix should not revert to the buggy behavior.
The actual fix is to make the RC follow ABI stability to not break existing clients with old toolchains

@EthanHumphrey
Copy link
Author

EthanHumphrey commented Sep 17, 2024

Agreed with both points. I wrote this PR before discovering the original fix and the reasoning behind it. I'm in favor of fixing this to match the IEEE standard, but most concerned about older applications that might behave in weird ways without an update. The consequences could be very visible (in our app, all negatives turned positive), or could caused unexpected behavior in business logic not immediately clear to users. With iOS 18 available as of yesterday and it still not resolving this issue, I guess we will see quite soon what kind of impact it has.

Once a path forward is identified I'd love to update the PR for the experience.

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.

Decimal(sign:exponent:significand:) causes unexpected sign if significand is negative
4 participants