FAQ
Reviewers: gri1, rsc1, gri, remyoudompheng, rsc,

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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 faster
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

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 14, '12 at 11:22p
activeDec 14, '12 at 11:22p
posts1
users1
websitegolang.org

1 user in discussion

Adonovan: 1 post

People

Translate

site design / logo © 2022 Grokbase