Repository navigation
apd: fix Modf, Floor and Ceil when the result aliases the operand - #155
Merged
Merged
Conversation
Modf wrote the integral part's sign/exponent (or the zero value) into integ before reading d, so calling it with integ == d (as Floor and Ceil do) clobbered the operand. For |x| < 0.1 this made the fractional part zero, so Floor(-0.01) and Ceil(0.01) returned 0 instead of -1 and 1 when the result aliased the operand. Fixes cockroachdb#154.
2 tasks done
Modf's write order matters: integ or frac may be d itself, so every read of d happens before either result is written. Say so in the godoc and at the trailing write, note that integ and frac must be distinct, and fix the comment on the |d| < 1 branch, which said d < 0. Extend TestModf to the x.Modf(x, nil) and x.Modf(nil, x) forms and make TestCeil and TestFloor check that the aliased result matches a separate destination. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
rafiss
approved these changes
Oct 22, 2026
rafiss
left a comment
rafiss
left a comment
Contributor
There was a problem hiding this comment.
The reordering is correct and minimal. I traced all three branches for integ == d and frac == d, and the new tests fail on master for exactly the #154 cases.
I pushed 400d13c on top: it states in the Modf godoc that integ or frac may alias d and that the two must be distinct, fixes the "d < 0" comment on the |d| < 1 branch, notes at the trailing write why it comes after the reads, adds the x.Modf(x, nil) and x.Modf(nil, x) forms to TestModf, and makes TestCeil and TestFloor check the aliased result against a separate destination.
Floor and Ceil of NaN or Infinity still return a finite 0 into a non-aliased destination. That predates this PR and is being handled in #158.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #154.
Decimal.Modfsetinteg's exponent and sign (or zeroed it outright in the|d| < 1-with-leading-zeros branch) before it readd. Wheninteg == d, which is howFloorandCeilcall it in the aliased case, that overwrote the operand. For values like-0.01the fractional part then came back as zero, soFloor(x, x)returned-0instead of-1, andCeil(x, x)for0.01returned0instead of1. Thefrac == dcase had the same problem in theExponent > 0branch.The fix only reorders the writes. Every read of
dnow happens beforeinteg/fracare modified. The computation itself is unchanged.Tests:
TestFloor/TestCeil: added-0.01/0.01(they already run aliased,Floor(d, d)).TestModf: every existing case now also runs withintegaliased toxand withfracaliased tox.Validation (go1.23.4, linux/amd64):
TestCeil/0.01: got 0, expected 1andTestFloor/-0.01: got 0, expected -1.go test ./...ok,go test -race ./...ok,go vet -unsafeptr=false ./...clean.