Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(27)

Issue 6930059: code review 6930059: math/big: add Rat.{,Set}Float64 methods for IEEE 754 co... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by adonovan
Modified:
9 years, 8 months ago
Reviewers:
CC:
remyoudompheng, rsc, golang-dev
Visibility:
Public.

Description

math/big: add Rat.{,Set}Float64 methods for IEEE 754 conversions. Added tests, using input data from strconv.ParseFloat. Thanks to rsc for most of the test code. math/big could use some good package-level documentation.

Patch Set 1 #

Patch Set 2 : diff -r 2888e5323790 https://code.google.com/p/go/ #

Total comments: 41

Patch Set 3 : diff -r 2888e5323790 https://code.google.com/p/go/ #

Total comments: 19

Patch Set 4 : diff -r 3ef1c4fabb5f https://code.google.com/p/go/ #

Total comments: 64

Patch Set 5 : diff -r 3ef1c4fabb5f https://code.google.com/p/go/ #

Patch Set 6 : diff -r 3ef1c4fabb5f https://code.google.com/p/go/ #

Patch Set 7 : diff -r 3ef1c4fabb5f https://code.google.com/p/go/ #

Patch Set 8 : diff -r 3ef1c4fabb5f https://code.google.com/p/go/ #

Patch Set 9 : diff -r 3ef1c4fabb5f https://code.google.com/p/go/ #

Patch Set 10 : diff -r 601795ce6319 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+555 lines, -0 lines) Patch
M src/pkg/math/big/rat.go View 1 2 3 4 5 6 7 8 9 2 chunks +151 lines, -0 lines 0 comments Download
M src/pkg/math/big/rat_test.go View 1 2 3 4 5 6 7 8 2 chunks +404 lines, -0 lines 0 comments Download

Messages

Total messages: 19
gri
Haven't thought through this is detail yet, but here some prelim. feedback. Please pay attention ...
9 years, 9 months ago (2012-12-13 01:52:05 UTC) #1
remyoudompheng
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#newcode53 src/pkg/math/big/rat.go:53: exp := int((bits>>52)&0x7FF) - 1023 mantissa := bits & ...
9 years, 9 months ago (2012-12-13 07:37:38 UTC) #2
remyoudompheng
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#newcode82 src/pkg/math/big/rat.go:82: // The result contains mantissa A/B and exponent e. ...
9 years, 9 months ago (2012-12-13 07:44:05 UTC) #3
rsc
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 ...
9 years, 9 months ago (2012-12-13 14:41:58 UTC) #4
adonovan
Thanks for the helpful review comments---there were indeed a number of corner cases the tests ...
9 years, 9 months ago (2012-12-14 23:22:43 UTC) #5
remyoudompheng
https://codereview.appspot.com/6930059/diff/11001/src/pkg/math/big/nat.go File src/pkg/math/big/nat.go (right): https://codereview.appspot.com/6930059/diff/11001/src/pkg/math/big/nat.go#newcode116 src/pkg/math/big/nat.go:116: return uint64(z[1]<<32 | z[0]) on 32 bits, z[1]<<32 is ...
9 years, 9 months ago (2012-12-15 00:17:48 UTC) #6
rsc
Once Remy is happy I'll be happy. :-) 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 ...
9 years, 9 months ago (2012-12-15 02:49:06 UTC) #7
adonovan
Thanks again for the great review comments, and apologies for the lack of polish. If ...
9 years, 9 months ago (2012-12-15 20:43:46 UTC) #8
rsc
I am a little worried about the double rounding. I think it would be more ...
9 years, 9 months ago (2012-12-17 17:00:33 UTC) #9
rsc
3) should say ... are nonzero.
9 years, 9 months ago (2012-12-17 17:00:54 UTC) #10
adonovan
Hello remyoudompheng@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
9 years, 8 months ago (2013-01-25 21:08:45 UTC) #11
adonovan
https://codereview.appspot.com/6930059/diff/11001/src/pkg/math/big/nat.go File src/pkg/math/big/nat.go (right): https://codereview.appspot.com/6930059/diff/11001/src/pkg/math/big/nat.go#newcode116 src/pkg/math/big/nat.go:116: return uint64(z[1]<<32 | z[0]) On 2012/12/15 00:17:48, remyoudompheng wrote: ...
9 years, 8 months ago (2013-01-25 21:09:39 UTC) #12
rsc
Looks correct to me! https://codereview.appspot.com/6930059/diff/18001/src/pkg/math/big/rat.go File src/pkg/math/big/rat.go (right): https://codereview.appspot.com/6930059/diff/18001/src/pkg/math/big/rat.go#newcode32 src/pkg/math/big/rat.go:32: // If f is not ...
9 years, 8 months ago (2013-01-25 22:23:50 UTC) #13
adonovan
PTAL. Russ, thanks again for the patient and thorough review during which you effectively wrote ...
9 years, 8 months ago (2013-01-28 18:05:46 UTC) #14
rsc
I think you should decide whether you want the Float64 method to ever return -0. ...
9 years, 8 months ago (2013-01-28 18:15:07 UTC) #15
adonovan
On 2013/01/28 18:15:07, rsc wrote: > I think you should decide whether you want the ...
9 years, 8 months ago (2013-01-28 18:52:00 UTC) #16
adonovan
I think Float64() should return -0 in the case of small but nonzero negative numbers, ...
9 years, 8 months ago (2013-01-28 18:54:02 UTC) #17
rsc
LGTM
9 years, 8 months ago (2013-01-28 22:54:23 UTC) #18
adonovan
9 years, 8 months ago (2013-01-28 23:00:27 UTC) #19
*** Submitted as https://code.google.com/p/go/source/detail?r=ca5e5de48173 ***

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

Added tests, using input data from strconv.ParseFloat.
Thanks to rsc for most of the test code.

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

R=remyoudompheng, rsc
CC=golang-dev
https://codereview.appspot.com/6930059
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b