Message:

Thanks for the helpful review comments---there were indeed a number of

corner cases the tests hadn't touched.

Still one testcase fails, for the exact same reason described in the

Java blog post referenced. Suggestions welcome.

Ready for another look.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go

File src/pkg/math/big/rat.go (right):

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode35

src/pkg/math/big/rat.go:35: panic("SetFloat requires finite value")

On 2012/12/13 01:52:06, gri wrote:

SetFloat64

Done.SetFloat64

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode35

src/pkg/math/big/rat.go:35: panic("SetFloat requires finite value")

On 2012/12/13 01:52:06, gri wrote:

on that note, I am wondering if this is a good choice. alternatively, one might

return nil. as it is, a client will have to check anyway, so why not

provide the

result? also, if the client doesn't pay attention, it will die with a null ptr

exception when the result is used. about the same.

so: return nil ?

Done.on that note, I am wondering if this is a good choice. alternatively, one might

return nil. as it is, a client will have to check anyway, so why not

provide the

result? also, if the client doesn't pay attention, it will die with a null ptr

exception when the result is used. about the same.

so: return nil ?

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode37

src/pkg/math/big/rat.go:37: if f == 0.0 {

On 2012/12/13 01:52:06, gri wrote:

f == 0

seems good enough - we are in Go after all

Done.f == 0

seems good enough - we are in Go after all

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode38

src/pkg/math/big/rat.go:38: z.a, z.b = Int{}, *intOne

On 2012/12/13 01:52:06, gri wrote:

that's not how we do this in this package - you throw away whatever memory is

hanging of z by doing this

use the appropriate Set function

Done.that's not how we do this in this package - you throw away whatever memory is

hanging of z by doing this

use the appropriate Set function

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode43

src/pkg/math/big/rat.go:43: const SmallestNormal =

2.2250738585072014e-308 // 2**-1022

Per Remy's suggestion, this is gone.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode52

src/pkg/math/big/rat.go:52: mantissa := (1 << 52) | (bits &

0xFFFFFFFFFFFFF)

I tried that but it looked awful: long names everywhere, obscuring the

meaning. 11, 52 and 1023 need no explanation.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode53

src/pkg/math/big/rat.go:53: exp := int((bits>>52)&0x7FF) - 1023

Very nice. Done.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode57

src/pkg/math/big/rat.go:57: z.b = *intOne

On 2012/12/13 01:52:06, gri wrote:

use Set function so we don't throw away underlying memory

Done.use Set function so we don't throw away underlying memory

This package could use some documentation explaining the proper use. I

found I had to think quite hard about aliasing to avoid making even more

mistakes. That you have to mention a value up to four times in a binary

operation z = z.op(z, z) can be a cause for some head-scratching.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode60

src/pkg/math/big/rat.go:60: z.b.abs = z.b.abs.shl(z.b.abs, uint(shift))

On 2012/12/13 01:52:06, gri wrote:

use set functions

Done.use set functions

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode69

src/pkg/math/big/rat.go:69: // Preconditions: b is nonzero; a and b have

no common factors.

On 2012/12/13 01:52:06, gri wrote:

s/nonzero/non-zero/

Done.s/nonzero/non-zero/

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode82

src/pkg/math/big/rat.go:82: // The result contains mantissa A/B and

exponent e.

You're right, it was always finding a value of equal or smaller

magnitude, not the nearest.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode97

src/pkg/math/big/rat.go:97: expt := alen - blen

On 2012/12/13 01:52:06, gri wrote:

s/expt/exp/ please

Done.s/expt/exp/ please

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode117

src/pkg/math/big/rat.go:117: if expt > 1024 {

Done. (FWIW, that's "return -0.0" in effect.)

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode146

src/pkg/math/big/rat.go:146: mantissa := uint64(q[0] & 0xfffffffffffff)

// 52 bits

On 2012/12/13 07:37:38, remyoudompheng wrote:

This line doesn't compile at all.

Here Word is 32 bit.

Good catch; I'm testing on 64 bits. Fixed.This line doesn't compile at all.

Here Word is 32 bit.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode148

src/pkg/math/big/rat.go:148: (uint64((expt+1023)&0x7FF) << 52) |

mantissa)

On 2012/12/13 01:52:06, gri wrote:

one line please

Done.one line please

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode165

src/pkg/math/big/rat.go:165: exact = false

On 2012/12/13 01:52:06, gri wrote:

just return

Done.just return

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode172

src/pkg/math/big/rat.go:172: exact = z2.Cmp(z) == 0

On 2012/12/13 01:52:06, gri wrote:

I'd do:

if z2.Cmp(z) == 0 {

exact = true

}

Done.I'd do:

if z2.Cmp(z) == 0 {

exact = true

}

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat_test.go

File src/pkg/math/big/rat_test.go (right):

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat_test.go#newcode508

src/pkg/math/big/rat_test.go:508: // TODO(adonovan): is this ok? Or

should I copy the file, or even

On 2012/12/13 14:41:58, rsc wrote:

You can read the file directly, that's fine. As for using it, what I had

intended (but not expressed well) was to use the decimal -> binary

conversions

as rat -> binary conversions by building fractions with appropriate

denominators. For example the file contains

# Table 1: Stress Inputs for Conversion to 53-bit Binary, < 1/2 ULP

float64 %b 5e+125 6653062250012735p+365

float64 %b 69e+267 4705683757438170p+841

float64 %b 999e-026 6798841691080350p-129

In the strconv package that says to try strconv.ParseFloat("5e+125", 64) and

make sure you get the 6653062250012735*2³⁶⁵. Here, though, you could

create the

fraction 5*10¹²⁵ / 1 and call .Float64 on it and make sure you get

5e+125 out

(you can trust ParseFloat64, you don't need to look at the final field at all).

Similarly, a few lines later, you could create the fraction

836168422905420598437 / 10²³⁴ and call .Float64 on it and make sure

you get what

ParseFloat says for 836168422905420598437e-234.

Do the same with the strings in the table in

src/pkg/strconv/atof_test.go.

If the code can pass both of those sets of cases, then I'll be pretty confident

it's right.

Done.You can read the file directly, that's fine. As for using it, what I had

intended (but not expressed well) was to use the decimal -> binary

conversions

as rat -> binary conversions by building fractions with appropriate

denominators. For example the file contains

# Table 1: Stress Inputs for Conversion to 53-bit Binary, < 1/2 ULP

float64 %b 5e+125 6653062250012735p+365

float64 %b 69e+267 4705683757438170p+841

float64 %b 999e-026 6798841691080350p-129

In the strconv package that says to try strconv.ParseFloat("5e+125", 64) and

make sure you get the 6653062250012735*2³⁶⁵. Here, though, you could

create the

fraction 5*10¹²⁵ / 1 and call .Float64 on it and make sure you get

5e+125 out

(you can trust ParseFloat64, you don't need to look at the final field at all).

Similarly, a few lines later, you could create the fraction

836168422905420598437 / 10²³⁴ and call .Float64 on it and make sure

you get what

ParseFloat says for 836168422905420598437e-234.

Do the same with the strings in the table in

src/pkg/strconv/atof_test.go.

If the code can pass both of those sets of cases, then I'll be pretty confident

it's right.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/bits.go

File src/pkg/math/bits.go (right):

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/bits.go#newcode40

src/pkg/math/bits.go:40: func IsFinite(f float64) bool {

On 2012/12/13 01:52:06, gri wrote:

why not use IsInf?

This predicate is equivalent to !(IsNaN(f) || IsInf(f,0)) but fasterwhy not use IsInf?

than either arm. I've clarified this in the docstring.

Description:

math/big: add Rat.{,Set}Float64 methods for IEEE 754 conversions.

Added tests, using input data from strconv.ParseFloat .

Also added math.IsFinite, which is faster than

!IsNan(x) && !IsInf(x,0). Plus tests.

math/big could use some good package-level documentation.

Please review this at https://codereview.appspot.com/6930059/

Affected files:

M src/pkg/math/all_test.go

M src/pkg/math/big/nat.go

M src/pkg/math/big/rat.go

M src/pkg/math/big/rat_test.go

M src/pkg/math/bits.go