Skip to content

Commit

Permalink
internal/number: fix more rounding issues
Browse files Browse the repository at this point in the history 
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 <mpvl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
  • Loading branch information
@mpvl
mpvl committed Sep 1, 2017
1 parent 5d3456f commit 5c11b69
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 43 deletions.
20 changes: 12 additions & 8 deletions internal/number/decimal.go
View file
Expand Up @@ -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
Expand Down
19 changes: 11 additions & 8 deletions internal/number/decimal_test.go
View file
Expand Up @@ -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"},
Expand All @@ -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"},

Expand Down
49 changes: 26 additions & 23 deletions internal/number/format.go
View file
Expand Up @@ -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)
}
Expand All @@ -210,7 +209,6 @@ func decimalVisibleDigits(r RoundingContext, d *Decimal) Digits {
n.End = int32(r.MinSignificantDigits)
}
}
n.Digits = digits
n.Exp = exp
return n
}
Expand Down Expand Up @@ -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))
Expand Down
8 changes: 4 additions & 4 deletions internal/number/format_test.go
View file
Expand Up @@ -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": "∞",
},
Expand Down Expand Up @@ -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",
},
}, {
Expand All @@ -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",
},

Expand Down Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions internal/number/pattern.go
View file
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions internal/number/pattern_test.go
View file
Expand Up @@ -203,6 +203,7 @@ var testCases = []struct {
RoundingContext: RoundingContext{
MinSignificantDigits: 1,
MaxSignificantDigits: 1,
MaxFractionDigits: -1,
},
},
}, {
Expand All @@ -213,6 +214,7 @@ var testCases = []struct {
RoundingContext: RoundingContext{
MinSignificantDigits: 4,
MaxSignificantDigits: 4,
MaxFractionDigits: -1,
},
},
}, {
Expand All @@ -222,6 +224,7 @@ var testCases = []struct {
RoundingContext: RoundingContext{
MinSignificantDigits: 1,
MaxSignificantDigits: 4,
MaxFractionDigits: -1,
},
},
}, {
Expand Down

0 comments on commit 5c11b69

Please sign in to comment.