From 5c11b696d00c33495a190cbaf83ab94f9898c4bf Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Thu, 31 Aug 2017 16:33:48 +0200 Subject: [PATCH] internal/number: fix more rounding issues Removed AppendFloat-based rounding to not hit bug golang/go#21714. Fixed several edge cases and implemented some missing features that were exposed as a result of having to fall back on our own rounding. This also fixed some previously unnoticed bugs. Change-Id: Id675bdc9a9099eda0b9c31e5166b510b714887b5 Reviewed-on: https://go-review.googlesource.com/60730 Run-TryBot: Marcel van Lohuizen TryBot-Result: Gobot Gobot Reviewed-by: Nigel Tao --- internal/number/decimal.go | 20 ++++++++------ internal/number/decimal_test.go | 19 +++++++------ internal/number/format.go | 49 +++++++++++++++++---------------- internal/number/format_test.go | 8 +++--- internal/number/pattern.go | 1 + internal/number/pattern_test.go | 3 ++ 6 files changed, 57 insertions(+), 43 deletions(-) diff --git a/internal/number/decimal.go b/internal/number/decimal.go index ac2098db1..62074e7d7 100644 --- a/internal/number/decimal.go +++ b/internal/number/decimal.go @@ -411,14 +411,18 @@ func (d *Decimal) ConvertFloat(r RoundingContext, x float64, size int) { // ToNearestEven. // Something like this would work: // AppendDigits(dst []byte, x float64, base, size, prec int) (digits []byte, exp, accuracy int) - if r.Mode == ToNearestEven { - if n := r.RoundSignificantDigits(); n >= 0 { - prec = n - } else if n = r.RoundFractionDigits(); n >= 0 { - prec = n - verb = 'f' - } - } + // + // TODO: At this point strconv's rounding is imprecise to the point that it + // is not useable for this purpose. + // See https://github.com/golang/go/issues/21714 + // if r.Mode == ToNearestEven { + // if n := r.RoundSignificantDigits(); n >= 0 { + // prec = n + // } else if n = r.RoundFractionDigits(); n >= 0 { + // prec = n + // verb = 'f' + // } + // } b := strconv.AppendFloat(d.Digits[:0], abs, verb, prec, size) i := 0 diff --git a/internal/number/decimal_test.go b/internal/number/decimal_test.go index f13b7c194..04aa8b2c8 100644 --- a/internal/number/decimal_test.go +++ b/internal/number/decimal_test.go @@ -256,6 +256,13 @@ func TestConvert(t *testing.T) { rc RoundingContext out string }{ + // TODO: uncommented tests can be restored when convert does its own + // rounding. + // {-0.001, scale2, "-0.00"}, // not normalized + // {0.1234, prec3, "0.123"}, + // {1234.0, prec3, "1230"}, + // {1.2345e10, prec3, "12300000000"}, + {int8(-34), scale2, "-34"}, {int16(-234), scale2, "-234"}, {int32(-234), scale2, "-234"}, @@ -266,22 +273,18 @@ func TestConvert(t *testing.T) { {uint32(234), scale2, "234"}, {uint64(234), scale2, "234"}, {uint(234), scale2, "234"}, - {-0.001, scale2, "-0.00"}, // not normalized - {-1e9, scale2, "-1000000000.00"}, + {-1e9, scale2, "-1000000000"}, {0.234, scale2away, "0.234"}, // rounding postponed as not ToNearestEven - {0.1234, prec3, "0.123"}, - {1234.0, prec3, "1230"}, - {1.2345e10, prec3, "12300000000"}, {0.03, inc0_05, "0.05"}, - {0.025, inc0_05, "0.00"}, // not normalized - {0.075, inc0_05, "0.10"}, + {0.025, inc0_05, "0"}, + {0.075, inc0_05, "0.1"}, {325, inc50, "300"}, {375, inc50, "400"}, // Here the scale is 2, but the digits get shifted left. As we use // AppendFloat to do the rounding an exta 0 gets added. - {0.123, roundShift, "0.1230"}, + {0.123, roundShift, "0.123"}, {converter(3), scale2, "100"}, diff --git a/internal/number/format.go b/internal/number/format.go index 410478962..0a5ffb548 100755 --- a/internal/number/format.go +++ b/internal/number/format.go @@ -154,48 +154,47 @@ func decimalVisibleDigits(r RoundingContext, d *Decimal) Digits { } n := Digits{digits: d.normalize().digits} - if maxSig := int(r.MaxSignificantDigits); maxSig > 0 { - // TODO: really round to zero? - n.round(ToZero, maxSig) - } - digits := n.Digits exp := n.Exp exp += int32(r.DigitShift) // Cap integer digits. Remove *most-significant* digits. if r.MaxIntegerDigits > 0 { if p := int(exp) - int(r.MaxIntegerDigits); p > 0 { - if p > len(digits) { - p = len(digits) + if p > len(n.Digits) { + p = len(n.Digits) } - if digits = digits[p:]; len(digits) == 0 { + if n.Digits = n.Digits[p:]; len(n.Digits) == 0 { exp = 0 } else { exp -= int32(p) } // Strip leading zeros. - for len(digits) > 0 && digits[0] == 0 { - digits = digits[1:] + for len(n.Digits) > 0 && n.Digits[0] == 0 { + n.Digits = n.Digits[1:] exp-- } } } - // Rounding usually is done by convert, but we don't rely on it. - numFrac := len(digits) - int(exp) - if r.MaxSignificantDigits == 0 && int(r.MaxFractionDigits) < numFrac { - p := int(exp) + int(r.MaxFractionDigits) - if p <= 0 { + // Rounding if not already done by Convert. + p := len(n.Digits) + if maxSig := int(r.MaxSignificantDigits); maxSig > 0 { + p = maxSig + } + if maxFrac := int(r.MaxFractionDigits); maxFrac >= 0 { + if cap := int(exp) + maxFrac; cap < p { + p = int(exp) + maxFrac + } + if p < 0 { p = 0 - } else if p >= len(digits) { - p = len(digits) } - digits = digits[:p] // TODO: round } + n.round(r.Mode, p) // set End (trailing zeros) - n.End = int32(len(digits)) - if len(digits) == 0 { + n.End = int32(len(n.Digits)) + if n.End == 0 { + exp = 0 if r.MinFractionDigits > 0 { n.End = int32(r.MinFractionDigits) } @@ -210,7 +209,6 @@ func decimalVisibleDigits(r RoundingContext, d *Decimal) Digits { n.End = int32(r.MinSignificantDigits) } } - n.Digits = digits n.Exp = exp return n } @@ -323,9 +321,14 @@ func scientificVisibleDigits(r RoundingContext, d *Decimal) Digits { numInt += d } - if maxSig := int(r.MaxFractionDigits); maxSig >= 0 { - n.round(r.Mode, maxSig+numInt) + p := len(n.Digits) + if maxSig := int(r.MaxSignificantDigits); maxSig > 0 { + p = maxSig + } + if maxFrac := int(r.MaxFractionDigits); maxFrac >= 0 && numInt+maxFrac < p { + p = numInt + maxFrac } + n.round(r.Mode, p) n.Comma = uint8(numInt) n.End = int32(len(n.Digits)) diff --git a/internal/number/format_test.go b/internal/number/format_test.go index 12738188e..01a089430 100755 --- a/internal/number/format_test.go +++ b/internal/number/format_test.go @@ -112,7 +112,7 @@ func TestAppendDecimal(t *testing.T) { test: pairs{ "0": "0", "1234.5678": "1234.5678", - "0.123456789": "0.123456", + "0.123456789": "0.123457", "NaN": "NaN", "Inf": "∞", }, @@ -142,7 +142,7 @@ func TestAppendDecimal(t *testing.T) { pattern: "#,##0.###", test: pairs{ "0": "0", - "1234.5678": "1,234.567", + "1234.5678": "1,234.568", "0.123456789": "0.123", }, }, { @@ -157,7 +157,7 @@ func TestAppendDecimal(t *testing.T) { test: pairs{ "0": "0,00,000", "123456789012": "1,23,45,67,89,012", - "12.3456789": "0,00,012.345", + "12.3456789": "0,00,012.346", "0.123456789": "0,00,000.123", }, @@ -245,7 +245,7 @@ func TestAppendDecimal(t *testing.T) { pattern: "@@##", test: pairs{ "1": "1.0", - "0.1": "0.10", + "0.1": "0.10", // leading zero does not count as significant digit "123": "123", "1234": "1234", "12345": "12340", diff --git a/internal/number/pattern.go b/internal/number/pattern.go index eebaafd24..b95ca40e8 100644 --- a/internal/number/pattern.go +++ b/internal/number/pattern.go @@ -358,6 +358,7 @@ func (p *parser) number(r rune) state { case '@': p.groupingCount++ p.leadingSharps = 0 + p.MaxFractionDigits = -1 return p.sigDigits(r) case ',': if p.leadingSharps == 0 { // no leading commas diff --git a/internal/number/pattern_test.go b/internal/number/pattern_test.go index 3a0b2be3f..a7517d004 100644 --- a/internal/number/pattern_test.go +++ b/internal/number/pattern_test.go @@ -203,6 +203,7 @@ var testCases = []struct { RoundingContext: RoundingContext{ MinSignificantDigits: 1, MaxSignificantDigits: 1, + MaxFractionDigits: -1, }, }, }, { @@ -213,6 +214,7 @@ var testCases = []struct { RoundingContext: RoundingContext{ MinSignificantDigits: 4, MaxSignificantDigits: 4, + MaxFractionDigits: -1, }, }, }, { @@ -222,6 +224,7 @@ var testCases = []struct { RoundingContext: RoundingContext{ MinSignificantDigits: 1, MaxSignificantDigits: 4, + MaxFractionDigits: -1, }, }, }, {