[SashaMIT]

Summary

Modf wrote the coefficient, exponent, and sign and left Form untouched. A destination that already held Infinity or NaN still stringified as that form. A fresh destination was already finite.

Modf of 1.5 into an Infinity and a NaN now returns 1 and 0.5. A value smaller than 1, such as 0.00001, has integral part 0 when that destination previously held Infinity. An integer such as 20 has fractional part 0 when that destination previously held NaN.

Ceil and Floor call toIntegralSpecials first, so Infinity and NaN stay themselves. A signaling NaN raises InvalidOperation.

Test plan

• TestModfResetsNonFiniteDestination
• TestCeilFloorNonFinite, aliased and fresh
• go test -count=1 . passed

[rafiss]

Thanks, the reused-destination bug is real and the new test covers it. One thing needs fixing before merge, plus a few small ones.

This makes Context.Ceil and Context.Floor return 0 for Infinity or NaN. Modf never checks d.Form, and an Inf or NaN parsed by SetString has coefficient 0 and exponent 0, so it goes down the QuoRem path. With the new integ.Form = Finite write, c.Ceil(x, x) and c.Floor(x, x) with x = Infinity, -Infinity, NaN or sNaN now return 0 (or -0) with no condition and no error. On master they returned the special value unchanged:

                      master       this PR
Ceil(x,x) Infinity  -> Infinity    0
Ceil(x,x) -Infinity -> -Infinity   -0
Ceil(x,x) NaN       -> NaN         0

Ceil(fresh, Infinity) already returned 0 on master, so this exposes an existing bug in the aliased form rather than creating it. Still, a silent finite result for a non-finite input is exactly what the Condition flags are there to prevent. The simplest fix is to have Context.Ceil and Context.Floor call c.toIntegralSpecials(d, x) first, like RoundToIntegralValue does a few lines up. That returns Inf unchanged, propagates NaN, raises InvalidOperation for sNaN, and fixes the fresh-destination case too. Please add Infinity, -Infinity, NaN and sNaN rows to TestCeil and TestFloor (aliased, which is how those tests already call them) plus a non-aliased variant. Nothing in the suite covers a non-finite receiver today.

After that, the doc sentence "Both results are finite." should say what's actually guaranteed, e.g. "d must be finite; both results are finite with Form set." Nothing checks it as written, and a hand-built Decimal{Form: Infinite, Exponent: 3} goes through integ.Set(d) and comes out Infinite.

The PR body says 0.00001 "now has integral part 0 instead of Infinity" and 20 "now has fractional part 0 instead of NaN", but doesn't say this only applies when the destination previously held Infinity or NaN. The commit message has it right.

Also, #155 reorders the writes in this function so Modf(d, &frac) works when integ == d. Both touch the same if integ != nil block, so one will need a rebase. If 155 lands first, drop the three lines at the conflict and put integ.Form = Finite next to the integ.Exponent = 0; integ.Negative = neg that 155 moves to the end of the function, after the QuoRem, so the reads of d still happen before any write.

[rafiss]

Nothing checks this, and it's false for a non-finite receiver (see above). I'd state the precondition instead: "d must be finite; both results are finite with Form set."

[rafiss]

This is the write that turns Ceil(x, x) of Infinity into 0, since an Inf/NaN has coefficient 0 and exponent 0 and lands here. The write is right for a finite d. The fix belongs in Context.Ceil/Floor via toIntegralSpecials so non-finite inputs never reach Modf.

[rafiss]

This describes the old bug in the present tense ("is still that form until Form is cleared"), and "the coefficient written below" doesn't refer to anything in the test. Maybe just "regression test: Modf into a destination that previously held Infinity or NaN must reset Form."

[mw5h]

I went through this at ee3b262 to check the review above experimentally and to see where it stands now that #155 has merged. Short version: the bug is real and still present on master, every point in the existing review checks out, and the PR now needs a rebase that is slightly more than mechanical.

Still a real bug on master. e9c7a71 has no Form write in Modf. TestModfResetsNonFiniteDestination copied onto master fails with integ = Infinity (form Infinite), want 1, and passes on this branch. #155 fixed a different Modf bug (aliasing), not this one. On master the same bug shows through Context.Ceil/Floor with a reused non-finite destination: Ceil(d := Infinity, 1.5) returns Infinity.

The fix itself is right for finite inputs. For every finite input I tried × destinations {fresh, Inf, -Inf, NaN, sNaN, stale 30-digit coefficient, stale coefficient with Form=Infinite}, all three direct-write paths produce correct Form/Negative/Coeff/Exponent/String, including signed zeros (Modf(-20) → frac -0, Modf(-0.5) → integ -0). Fresh-destination output is byte-identical to master on all 294 rows, which also confirms the point above about the PR body overstating the change. Explicit Form = Finite beside the field writes is the package convention (Context.add/Mul/Quo/QuoInteger/Rem do the same); there is no better helper.

Confirmed the Ceil/Floor concern. With x aliased: master returns Infinity / -Infinity / NaN / sNaN unchanged, this branch returns 0 / -0 / 0 / 0 with no Condition. Adding c.toIntegralSpecials(d, x) at the top of Ceil and Floor gives Infinity / -Infinity / NaN / NaN + InvalidOperation and also fixes Ceil(fresh, Infinity), which is already 0 on master. In CockroachDB the SQL floor()/ceil() builtins use a fresh destination (unchanged by this PR, and probably return 0 for 'Infinity'::DECIMAL today), jsonpath calls the aliased form (would change, but JSON can't carry Inf/NaN), and trunc() already guards Form before calling Modf.

The rebase: this branch fails master's TestModf. The PR is based on 6d9c587, before #155 moved the integ writes after every read of d. The hunk here carries the old order, so against master's decimal_test.go ten-plus TestModf subtests fail, e.g. TestModf/-1234.56e-2: integ aliased: got -12, -3456, expected -12, -0.3456. Concretely, on this branch:

x := 1.5;     x.Modf(x, &f)  →  f = 5        (master: 0.5)
x := 0.00001; x.Modf(x, &f)  →  f = 0        (master: 0.00001)

Not something the PR introduced (its base had the same order), but git merge master conflicts in decimal.go, and a resolution that keeps this hunk reintroduces the aliasing bug. The Form write's position is irrelevant (Modf never reads d.Form), so the rebased version is master's Modf verbatim plus exactly four lines, with the general-path integ.Form = Finite next to the integ.Exponent = 0; integ.Negative = neg block that #155 moved to the end:

 		if frac != nil {
+			frac.Form = Finite
 			frac.Negative = neg
 			frac.Exponent = 0
 			frac.Coeff.SetInt64(0)
@@
 		if integ != nil {
+			integ.Form = Finite
 			integ.Negative = neg
 			integ.Exponent = 0
 			integ.Coeff.SetInt64(0)
@@
 	if frac != nil {
 		icoeff.QuoRem(&d.Coeff, e, &frac.Coeff)
+		frac.Form = Finite
 		frac.Exponent = d.Exponent
 		frac.Negative = neg
@@
 	// These writes come after every read of d because integ may alias d.
 	if integ != nil {
+		integ.Form = Finite
 		integ.Exponent = 0
 		integ.Negative = neg
 	}

I built that plus the toIntegralSpecials guard in Ceil/Floor: full suite passes, master's aliasing rows pass, this PR's test passes, and aliased outputs are identical to master for all finite inputs. Also keep master's doc sentence about aliasing ("integ or frac may be d itself; integ and frac must not be the same Decimal") and master's |d| < 1 comment on the exp > nd branch, which a naive merge would revert to the old d < 0 typo. For tests, consider a dirty-destination variant inside the table-driven TestModf loop so every row (including the negative-zero ones) is exercised, rather than three hand-picked values.

[SashaMIT]

Thanks. Ceil and Floor now call toIntegralSpecials first, so Infinity and NaN stay themselves and a signaling NaN raises InvalidOperation. The rebase keeps the writes after every read of d, and sets Form to Finite on each result. The aliasing sentence stays. beca176. The package tests passed.