Conversation
rootSpecials divided the operand exponent by factor with Go's /, which truncates toward zero, so every odd negative exponent came out one too high. It also never brought the result into the context's exponent range and never raised Clamped, unlike quoSpecials. TestGDA compares finite results with Cmp, which treats every zero as equal regardless of exponent, so the vendored vectors never caught it.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d21033f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if x.Exponent%factor != 0 && x.Exponent < 0 { | ||
| e-- |
There was a problem hiding this comment.
Because rootSpecials is also called by Cbrt, flooring here changes positive cube-root results with negative, non-divisible exponents: for example, Cbrt("00.0") now returns 0.0 with exponent -1, while testdata/cuberoot-apd.decTest cases cbtx006–cbtx008 require exponent 0. TestGDA does not expose this regression because it compares zeros numerically, and the new targeted test only selects the contradictory negative-zero expectations. Restrict the flooring change to square root, or first define and consistently update and test the cube-root contract.
Useful? React with 👍 / 👎.
1 participant
SqrtandCbrtof a zero return the wrong exponent and never clamp.The repo's own vendored vector disagrees —
testdata/squareroot.decTest:3780sayssqtx9010 squareroot 0E-9 -> 0.00000, exponent −5, and Python'sdecimalagrees. Every odd negative exponent is off by one. The clamp is missing outright: withMaxExponent: 9,Sqrtof0E+100returns exponent 50 and noClamped.rootSpecials(context.go:462-465) doesd.Exponent /= factor, and Go's/truncates toward zero where the ideal exponent isfloor(e/factor); the result is then never brought into[etiny, MaxExponent]nor flagged. The siblingquoSpecials(context.go:247-250) already does both. I usedc.etiny()andc.goErrorrather thansetExponent, becausesetExponentalso raisesRounded, which these vectors do not expect.Why 22,915 conformance cases never caught it:
gda_test.go:714-715compares finite results withd.Cmp(r), which is numeric, so every zero compares equal to every other zero regardless of exponent. The decTest files carry the exponents; the comparison discards them.One thing to decide: Cbrt is collateral
rootSpecialsis shared, so this changesCbrttoo, andtestdata/cuberoot-apd.decTest— this repo's own hand-written file — is already inconsistent about zeros:Same operand exponent, contradictory expectations;
cbtx022 cuberoot 0E+5 -> 0also disagrees withcbtx017/cbtx020. None of it is enforced, for the sameCmpreason. I did not touch that data. Happy to correct those lines in a follow-up, or to restrict this tofactor == 2and leaveCbrtalone — your call.Verification
mastersqtx006x.Exponent < 0test)sqtx9037Clampedflagsqtx9024,got conditions "", expected "clamped"This lets 7 entries out of
GDAignoreFlags, checked both ways:TestGDAexits 0.TestGDAfails on exactly those 7 IDs. So the removals are earned by the fix, not by loosening the test.sqtx9045stays: removing it too still fails, because apd does not implement theclamp:directive. I corrected its comment to say so.CI rows on go1.26.3:
go test ./...,-race,-bench=. -benchtime=1x,go vet -unsafeptr=false ./...,staticcheck ./..., andgo test -cforGOARCH=arm GOARM=7andarm64— all exit 0. I did not run a real go1.17/1.19 toolchain; the diff uses only/,%and existing identifiers.gofmt -l .flags four files on pristine too (go1.26's gofmt reformats the//gcassert:comments), so I did not rungofmt -w; the diff contains only therootSpecialshunk.Disclosure: I used Claude (an AI assistant) while preparing this change. Every result above I ran and verified myself.