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

Issue 6344078: code review 6344078: math, runtime: use a NaN that matches gcc's (Closed)

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

Description

math, runtime: use a NaN that matches gcc's our old choice is not working properly at least on VFPv2 in ARM1136JF-S (it's not preserved across float64->float32 conversions). Fixes issue 3745.

Patch Set 1 #

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

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

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

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

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

Total comments: 2

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

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

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

Total comments: 1

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -2 lines) Patch
M src/pkg/math/all_test.go View 1 2 3 4 5 6 1 chunk +11 lines, -0 lines 0 comments Download
M src/pkg/math/bits.go View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/float.c View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 14
dfc
LGTM. Tested on arm5 and arm7. pkg/math benchmarks show no performance regression, but these two ...
9 years, 10 months ago (2012-07-04 02:17:35 UTC) #1
minux1
still need to test on ARMv6, I can only verify fmt and math test passed, ...
9 years, 10 months ago (2012-07-04 03:03:13 UTC) #2
minux1
added a test for NaN() in math pkg so that we can catch the problem ...
9 years, 10 months ago (2012-07-05 19:44:24 UTC) #3
dfc
On 2012/07/05 19:44:24, minux wrote: > added a test for NaN() in math pkg so ...
9 years, 10 months ago (2012-07-11 04:51:37 UTC) #4
minux1
any more comments on this CL?
9 years, 10 months ago (2012-07-25 21:34:50 UTC) #5
rsc
Please mention in the comments that we're talking about conversion to float32. I had to ...
9 years, 9 months ago (2012-07-29 22:17:47 UTC) #6
minux1
On Sun, Jul 29, 2012 at 3:17 PM, <rsc@golang.org> wrote: > I am not sure ...
9 years, 9 months ago (2012-07-30 00:01:04 UTC) #7
rsc
So you're suggesting to use the same nan bits on all platforms, and just change ...
9 years, 9 months ago (2012-07-30 00:48:29 UTC) #8
minux1
PTAL.
9 years, 9 months ago (2012-07-30 01:45:43 UTC) #9
rsc
LGTM http://codereview.appspot.com/6344078/diff/18004/src/pkg/math/bits.go File src/pkg/math/bits.go (right): http://codereview.appspot.com/6344078/diff/18004/src/pkg/math/bits.go#newcode8 src/pkg/math/bits.go:8: uvnan = 0x7ff8000000000001 please use capital letters like ...
9 years, 9 months ago (2012-08-03 18:03:13 UTC) #10
rsc
Update CL description too: drop "on ARM".
9 years, 9 months ago (2012-08-03 18:03:36 UTC) #11
minux1
Hello dave@cheney.net, 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, 9 months ago (2012-08-07 01:57:05 UTC) #12
minux1
*** Submitted as http://code.google.com/p/go/source/detail?r=1f62df249175 *** math, runtime: use a NaN that matches gcc's our old ...
9 years, 9 months ago (2012-08-07 01:57:30 UTC) #13
dfc
9 years, 9 months ago (2012-08-07 01:59:40 UTC) #14
outstanding.
Sign in to reply to this message.

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