code review 1442041: strconv.Ftoa: fixes a precision error

strconv.Ftoa: fixes a precision error

Ftoa64(float64(0.09), 'f', 1) should return "0.1", but returns "0.0"
With this changeset this is fixed, but now Atof64("4e-324") returns 5e-324 and not 0 as it used to.
printf("%.1g", 4e-324); prints 5e-324 in gcc 4.4.3 (glibc 2.10.1).
Fixes issue 822.

[danielfleischman, 2010-06-01 03:30:33]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2010-06-02 02:18:39]

Thanks for looking into this.
Could you please complete the CLA as described at
http://golang.org/doc/contribute.html#copyright ?

Thanks.
Russ


On Mon, May 31, 2010 at 20:30,  <danielfleischman@gmail.com> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> strconv.Ftoa: fixes a precision error
>
> Ftoa64(float64(0.09), 'f', 1) should return "0.1", but returns "0.0"
> With this changeset this is fixed, but now Atof64("4e-324") returns
> 5e-324 and not 0 as it used to.
> printf("%.1g", 4e-324); prints 5e-324 in gcc 4.4.3.
> Fixes issue 822.
>
> Please review this at http://codereview.appspot.com/1442041/show
>
> Affected files:
>  M src/pkg/strconv/atof_test.go
>  M src/pkg/strconv/decimal.go
>  M src/pkg/strconv/ftoa_test.go
>
>
> Index: src/pkg/strconv/atof_test.go
> ===================================================================
> --- a/src/pkg/strconv/atof_test.go
> +++ b/src/pkg/strconv/atof_test.go
> @@ -75,7 +75,7 @@
>        // smallest denormal
>        atofTest{"5e-324", "5e-324", nil},
>        // too small
> -       atofTest{"4e-324", "0", nil},
> +       atofTest{"4e-324", "5e-324", nil},
>        // way too small
>        atofTest{"1e-350", "0", nil},
>        atofTest{"1e-400000", "0", nil},
> Index: src/pkg/strconv/decimal.go
> ===================================================================
> --- a/src/pkg/strconv/decimal.go
> +++ b/src/pkg/strconv/decimal.go
> @@ -289,7 +289,7 @@
>
>  // If we chop a at nd digits, should we round up?
>  func shouldRoundUp(a *decimal, nd int) bool {
> -       if nd <= 0 || nd >= a.nd {
> +       if nd < 0 || nd >= a.nd {
>                return false
>        }
>        if a.d[nd] == '5' && nd+1 == a.nd { // exactly halfway - round to
> even
> @@ -302,7 +302,7 @@
>  // Round a to nd digits (or fewer).
>  // Returns receiver for convenience.
>  func (a *decimal) Round(nd int) *decimal {
> -       if nd <= 0 || nd >= a.nd {
> +       if nd < 0 || nd >= a.nd {
>                return a
>        }
>        if shouldRoundUp(a, nd) {
> @@ -314,7 +314,7 @@
>  // Round a down to nd digits (or fewer).
>  // Returns receiver for convenience.
>  func (a *decimal) RoundDown(nd int) *decimal {
> -       if nd <= 0 || nd >= a.nd {
> +       if nd < 0 || nd >= a.nd {
>                return a
>        }
>        a.nd = nd
> @@ -325,7 +325,7 @@
>  // Round a up to nd digits (or fewer).
>  // Returns receiver for convenience.
>  func (a *decimal) RoundUp(nd int) *decimal {
> -       if nd <= 0 || nd >= a.nd {
> +       if nd < 0 || nd >= a.nd {
>                return a
>        }
>
> Index: src/pkg/strconv/ftoa_test.go
> ===================================================================
> --- a/src/pkg/strconv/ftoa_test.go
> +++ b/src/pkg/strconv/ftoa_test.go
> @@ -97,6 +97,8 @@
>        ftoaTest{-math.Inf(0), 'g', -1, "-Inf"},
>
>        ftoaTest{-1, 'b', -1, "-4503599627370496p-52"},
> +
> +       ftoaTest{0.09, 'f', 1, "0.1"},
>  }
>
>  func TestFtoa(t *testing.T) {
>
>
>

[danielfleischman, 2010-06-02 04:29:47]

Done.

On Tue, Jun 1, 2010 at 23:18, Russ Cox <rsc@golang.org> wrote:

> Thanks for looking into this.
> Could you please complete the CLA as described at
> http://golang.org/doc/contribute.html#copyright ?
>
> Thanks.
> Russ
>
>
> On Mon, May 31, 2010 at 20:30,  <danielfleischman@gmail.com> wrote:
> > Reviewers: golang-dev_googlegroups.com,
> >
> > Message:
> > Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
> >
> > I'd like you to review this change.
> >
> >
> > Description:
> > strconv.Ftoa: fixes a precision error
> >
> > Ftoa64(float64(0.09), 'f', 1) should return "0.1", but returns "0.0"
> > With this changeset this is fixed, but now Atof64("4e-324") returns
> > 5e-324 and not 0 as it used to.
> > printf("%.1g", 4e-324); prints 5e-324 in gcc 4.4.3.
> > Fixes issue 822.
> >
> > Please review this at http://codereview.appspot.com/1442041/show
> >
> > Affected files:
> >  M src/pkg/strconv/atof_test.go
> >  M src/pkg/strconv/decimal.go
> >  M src/pkg/strconv/ftoa_test.go
> >
> >
> > Index: src/pkg/strconv/atof_test.go
> > ===================================================================
> > --- a/src/pkg/strconv/atof_test.go
> > +++ b/src/pkg/strconv/atof_test.go
> > @@ -75,7 +75,7 @@
> >        // smallest denormal
> >        atofTest{"5e-324", "5e-324", nil},
> >        // too small
> > -       atofTest{"4e-324", "0", nil},
> > +       atofTest{"4e-324", "5e-324", nil},
> >        // way too small
> >        atofTest{"1e-350", "0", nil},
> >        atofTest{"1e-400000", "0", nil},
> > Index: src/pkg/strconv/decimal.go
> > ===================================================================
> > --- a/src/pkg/strconv/decimal.go
> > +++ b/src/pkg/strconv/decimal.go
> > @@ -289,7 +289,7 @@
> >
> >  // If we chop a at nd digits, should we round up?
> >  func shouldRoundUp(a *decimal, nd int) bool {
> > -       if nd <= 0 || nd >= a.nd {
> > +       if nd < 0 || nd >= a.nd {
> >                return false
> >        }
> >        if a.d[nd] == '5' && nd+1 == a.nd { // exactly halfway - round to
> > even
> > @@ -302,7 +302,7 @@
> >  // Round a to nd digits (or fewer).
> >  // Returns receiver for convenience.
> >  func (a *decimal) Round(nd int) *decimal {
> > -       if nd <= 0 || nd >= a.nd {
> > +       if nd < 0 || nd >= a.nd {
> >                return a
> >        }
> >        if shouldRoundUp(a, nd) {
> > @@ -314,7 +314,7 @@
> >  // Round a down to nd digits (or fewer).
> >  // Returns receiver for convenience.
> >  func (a *decimal) RoundDown(nd int) *decimal {
> > -       if nd <= 0 || nd >= a.nd {
> > +       if nd < 0 || nd >= a.nd {
> >                return a
> >        }
> >        a.nd = nd
> > @@ -325,7 +325,7 @@
> >  // Round a up to nd digits (or fewer).
> >  // Returns receiver for convenience.
> >  func (a *decimal) RoundUp(nd int) *decimal {
> > -       if nd <= 0 || nd >= a.nd {
> > +       if nd < 0 || nd >= a.nd {
> >                return a
> >        }
> >
> > Index: src/pkg/strconv/ftoa_test.go
> > ===================================================================
> > --- a/src/pkg/strconv/ftoa_test.go
> > +++ b/src/pkg/strconv/ftoa_test.go
> > @@ -97,6 +97,8 @@
> >        ftoaTest{-math.Inf(0), 'g', -1, "-Inf"},
> >
> >        ftoaTest{-1, 'b', -1, "-4503599627370496p-52"},
> > +
> > +       ftoaTest{0.09, 'f', 1, "0.1"},
> >  }
> >
> >  func TestFtoa(t *testing.T) {
> >
> >
> >
>

[rsc1, 2010-06-02 23:38:45]

Thanks for fixing this.
I'm still not sure I understand where the bug is
or why those changes would fix it.  However, I tried
playing around with it and added some more tests.
If you add the tests below I think you'll find that 
the code crashes.  So something's not quite right.  ;-)

http://codereview.appspot.com/1442041/diff/2001/3001
File src/pkg/strconv/atof_test.go (right):

http://codereview.appspot.com/1442041/diff/2001/3001#newcode78
src/pkg/strconv/atof_test.go:78: atofTest{"4e-324", "5e-324", nil},
This was trying to be a different test (and failing).
Please move it up a line and then add two more tests,
so that the block looks like:

	// smallest denormal
	atofTest{"5e-324", "5e-324", nil},
	atofTest{"4e-324", "5e-324", nil},
	atofTest{"3e-324", "5e-324", nil},
	// too small
	atofTest{"2e-324", "0", nil},
	// way too small

http://codereview.appspot.com/1442041/diff/2001/3003
File src/pkg/strconv/ftoa_test.go (right):

http://codereview.appspot.com/1442041/diff/2001/3003#newcode101
src/pkg/strconv/ftoa_test.go:101: ftoaTest{0.09, 'f', 1, "0.1"},
This is a really embarrassing bug.
Could you add a few more tests?

	ftoaTest{0.9, 'f', 1, "0.9"},
	ftoaTest{0.09, 'f', 1, "0.1"},
	ftoaTest{0.0999, 'f', 1, "0.1"},
	ftoaTest{0.05, 'f', 1, "0.1"},
	ftoaTest{0.05, 'f', 0, "1"},
	ftoaTest{0.5, 'f', 1, "0.5"},
	ftoaTest{0.5, 'f', 0, "1"},

[danielfleischman, 2010-06-03 00:41:50]

On Wed, Jun 2, 2010 at 20:38, <rsc@google.com> wrote:
> Thanks for fixing this.
:-)
> I'm still not sure I understand where the bug is
> or why those changes would fix it.
In the current code, if you try to round "0.09" to 1 decimal case, ftoa will
call round with nd=0, and shouldRoundUp will return false. With this, it checks
the a.d[0] digit.
> However, I tried
> playing around with it and added some more tests.
> If you add the tests below I think you'll find that
> the code crashes.  So something's not quite right.  ;-)
This was a great mistake. I have already fixed it. Thanks for pointing out.

http://codereview.appspot.com/1442041/diff/2001/3001
File src/pkg/strconv/atof_test.go (right):

http://codereview.appspot.com/1442041/diff/2001/3001#newcode78
src/pkg/strconv/atof_test.go:78: atofTest{"4e-324", "5e-324", nil},
On 2010/06/02 23:38:45, rsc1 wrote:
> This was trying to be a different test (and failing).
> Please move it up a line and then add two more tests,
> so that the block looks like:
> 
> 	// smallest denormal
> 	atofTest{"5e-324", "5e-324", nil},
> 	atofTest{"4e-324", "5e-324", nil},
> 	atofTest{"3e-324", "5e-324", nil},
> 	// too small
> 	atofTest{"2e-324", "0", nil},
> 	// way too small
> 
> 

Done.

http://codereview.appspot.com/1442041/diff/2001/3003
File src/pkg/strconv/ftoa_test.go (right):

http://codereview.appspot.com/1442041/diff/2001/3003#newcode101
src/pkg/strconv/ftoa_test.go:101: ftoaTest{0.09, 'f', 1, "0.1"},
Done, but I disagree with you in two of these tests.

On 2010/06/02 23:38:45, rsc1 wrote:
> This is a really embarrassing bug.
> Could you add a few more tests?
> 
> 	ftoaTest{0.9, 'f', 1, "0.9"},
> 	ftoaTest{0.09, 'f', 1, "0.1"},
> 	ftoaTest{0.0999, 'f', 1, "0.1"},
> 	ftoaTest{0.05, 'f', 1, "0.1"},
> 	ftoaTest{0.05, 'f', 0, "1"},
The last test should result "0" and not "1"
> 	ftoaTest{0.5, 'f', 1, "0.5"},
> 	ftoaTest{0.5, 'f', 0, "1"},
The last test should result "0" and not "1", since we have to round to the
nearest even number
> 
> 

This last case crashed my code, but it was a silly mistake. Sorry about missing
that and thanks for the test case.

[danielfleischman, 2010-06-03 00:46:06]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[danielfleischman, 2010-06-03 00:51:42]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[danielfleischman, 2010-06-03 00:54:03]

The last review was only to add one more test case that I think is
pertinent.

On Wed, Jun 2, 2010 at 21:51, <danielfleischman@gmail.com> wrote:

> Hello rsc (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/1442041/show
>

[danielfleischman, 2010-06-06 07:45:22]

Following up.

On Wed, Jun 2, 2010 at 21:53, Daniel Fleischman
<danielfleischman@gmail.com>wrote:

> The last review was only to add one more test case that I think is
> pertinent.
>
> On Wed, Jun 2, 2010 at 21:51, <danielfleischman@gmail.com> wrote:
>
>> Hello rsc (cc: golang-dev@googlegroups.com),
>>
>> Please take another look.
>>
>>
>> http://codereview.appspot.com/1442041/show
>>
>
>

[danielfleischman, 2010-06-15 10:26:21]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-06-29 23:49:05]

Thanks for figuring this out.
Rob and I were working on a different
floating point bug and incorporated your
patch (with credit) into 1738047.
To get your client back into
a good state you'll probably
want to run

hg revert @1442041
hg change -d 1442041
hg sync

Thanks again for tracking this down.
It took us a while to puzzle through
*why* it was the correct fix, but it was
a big help not to have to find the fix!

Russ

[rsc1, 2010-07-29 01:19:21]

(removing reviewers)