strconv: fix ParseFloat exponent and overflow bookkeeping - #129
Conversation
Four independent errors made ParseFloat return the wrong value: - mantExp subtracted one for a '.' that does not lie between trunk and dot, so truncating inside the integer part dropped one digit too many and any such literal with a dot came out 10x too large. - The uint64 overflow guard only checked MaxUint64/10 < n, missing the second clause ParseUint and ParseInt already have, so n wrapped and 18446744073709551616 parsed as 0. - The int * 10^k path scaled f and exp before deciding it could not return, so the fall-through applied the exponent twice. - Scaling by 10^-mantExp and 10^expExp separately let either factor leave math.Pow10's domain, so in-range values underflowed to 0 or overflowed to +Inf, and a zero mantissa produced NaN. Compared against strconv.ParseFloat over 381k generated literals no result that was already bit-exact changes.
| return f, i | ||
| } | ||
| h := f * math.Pow10(int(-mantExp)) | ||
| h *= math.Pow10(int(expExp)) |
There was a problem hiding this comment.
They could be merged mathematically, but the two-step is deliberate: for inputs like 1e-289 one of the factors stays inside math.Pow10's [-323,308] domain while the combined exponent does not. I measured the one-step variant — it turns 11,118 currently bit-exact results 1 ulp wrong — so the fix keeps the two-step and only re-scales when the result degenerates to 0/Inf.
| } | ||
| h := f * math.Pow10(int(-mantExp)) | ||
| h *= math.Pow10(int(expExp)) | ||
| if h == 0.0 || math.IsInf(h, 0) { |
There was a problem hiding this comment.
Didn't we check for 0.0 above with f==0.0?
There was a problem hiding this comment.
Both checks are needed: f == 0.0 catches a zero mantissa, but h == 0.0 also fires when f != 0 and the 10^-mantExp factor underflows (e.g. 5e-324 → 5 * 10^0 * 10^-324 = 0), which is exactly the case the re-scale rescues.
| if 0 <= exp && exp < 23 { | ||
| return f * float64pow10[exp], i | ||
| } else if 23 < exp && exp < 0 { | ||
| return f / float64pow10[exp], i |
There was a problem hiding this comment.
Let's fix this clause and do this like float, besides it moves more results closer according to your measurements.
There was a problem hiding this comment.
Done — the dead 23 < exp && exp < 0 branch is now the real negative fast path, mirroring ParseFloat: -22 <= exp && exp < 0 divides by the exact table power instead of multiplying by math.Pow10's inexact reciprocal. Measured over 300k structured inputs: 12,664 results closer to the correctly-rounded value vs 448 farther (e.g. 0.3 now parses to exactly 3/10). Regression test added; d6b77c8.
|
This is fantastic work, thank you very much for the effort! The changes look good to me, just a few questions that surged are above. Your contribution is highly appreciated! |
23 < exp && exp < 0 can never hold, so the negative-exponent fast path never ran; indexing float64pow10 with a negative exp would panic. Restore the intended branch as in ParseFloat (exp >= -22): dividing by an exact table power rounds closer than multiplying by math.Pow10's inexact reciprocal (12,664 closer vs 448 farther across 300k structured samples).
2e889e7 to
d6b77c8
Compare
|
Thank you very much! All merged |
2 participants
ParseFloat([]byte("99999999999999999999.0"))returns1e21instead of1e20, exactly a decade high: when the mantissa stops accumulating in the integer part,mantExpstill subtracts one for a'.'that isn't betweentrunkanddot, so it countsdot-trunk+1dropped digits instead ofdot-trunk. Three more wrong answers share that bookkeeping — the overflow guard is missing the second clauseParseUint/ParseIntgot in 2651d3a, so18446744073709551616wraps the accumulator and parses as0; theint * 10^kpath scalesfandexpbefore deciding to bail out, so the fall-through applies the exponent twice and993349238352373e23comes back 10x high; and the final scaling multiplies by10^-mantExpand10^expExpseparately, so either factor can leavemath.Pow10's[-323,308]domain, collapsing a fully written-out1e-289to0, turning0.0…1e400into+Infand0e309intoNaN.Measured against
strconv.ParseFloatover 381k generated literals: nothing the current code gets bit-exact changes, and 98k more become exact. The one-stepf * math.Pow10(int(exp))you'd expect here is deliberately not what I did — it fixes more but makes 11k currently-exact results 1 ulp worse, so the last step keeps the existing arithmetic and only re-scales when it degenerated to exactly0or±Inf. The residual gap to the stdlib is the intended 19-20 digit mantissa truncation, a few ulp, so the added random differential asserts a relative bound rather than equality;go test ./...stays green, and reverting just the two source files fails 20 of the new cases plus 70313 of 200000 random literals.This is reachable from minify's SVG path data (
svg/pathdata.go), where<path d="M18446744073709551616 0L1 1">currently minifies tom0 0L1 1, though it needs a longer literal than real path data usually carries. The second commit deletes aParseDecimalbranch whose23 < exp && exp < 0can never be true; writing it as intended (f / float64pow10[-exp], as float.go already does) moves 57344 of 400000 results closer to the stdlib but 11228 further, so I left that call to you.