Fix a bug that Math.round() returns incorrect results for huge integers.

Fix a bug that Math.round() returns incorrect results for huge integers.

[tkent, 2010-02-02 05:32:44]

# I'm not a committer of V8.

[Lasse Reichstein, 2010-02-02 08:52:29]

Very nice. I'll commit this.

http://codereview.chromium.org/567011/diff/1/3
File src/runtime.cc (right):

http://codereview.chromium.org/567011/diff/1/3#newcode4664
src/runtime.cc:4664: if (signbit(x) && x >= -0.5) return
Heap::minus_zero_value();
Well spotted.
The problem is entirely in the (+/-) 2^52+1 .. 2^53-1 range where the added 0.5
is immediately rounded using the default rounding, before we can do floor.
(Default rounding is banker's rounding - towards the even number).

http://codereview.chromium.org/567011/diff/1/3#newcode4666
src/runtime.cc:4666: return Heap::NumberFromDouble(integer - (integer - x >
0.5));
Apart from the implicit conversion from boolean to 0/1, this seems fine. I'll
add a conditional expression (.. ? 1 : 0).

http://codereview.chromium.org/567011/diff/1/2
File test/mjsunit/math-round.js (right):

http://codereview.chromium.org/567011/diff/1/2#newcode39
test/mjsunit/math-round.js:39: assertEquals(0, Math.round(-0.5));
Should actually be equal to -0, but our assertEquals doesn't distinguish 0 and
-0 (the usual way of testing (x == -0) is (1/x == -Infinity)).

http://codereview.chromium.org/567011/diff/1/2#newcode51
test/mjsunit/math-round.js:51: assertEquals(-Number.MAX_VALUE,
Math.round(-Number.MAX_VALUE));
Nice and thorough tests.

[tkent, 2010-02-02 09:12:15]

Thank you for reviewing. I have updated the patch.

http://codereview.chromium.org/567011/diff/1/3
File src/runtime.cc (right):

http://codereview.chromium.org/567011/diff/1/3#newcode4666
src/runtime.cc:4666: return Heap::NumberFromDouble(integer - (integer - x >
0.5));
On 2010/02/02 08:52:29, Lasse Reichstein wrote:
> Apart from the implicit conversion from boolean to 0/1, this seems fine. I'll
> add a conditional expression (.. ? 1 : 0).

I changed it to ?:.

http://codereview.chromium.org/567011/diff/1/2
File test/mjsunit/math-round.js (right):

http://codereview.chromium.org/567011/diff/1/2#newcode39
test/mjsunit/math-round.js:39: assertEquals(0, Math.round(-0.5));
On 2010/02/02 08:52:29, Lasse Reichstein wrote:
> Should actually be equal to -0, but our assertEquals doesn't distinguish 0 and
> -0 (the usual way of testing (x == -0) is (1/x == -Infinity)).

Done.

[Lasse Reichstein, 2010-02-02 09:20:12]

Whoops, that was quick response. 
Sorry for not being clearer: It was such a minor change, so would just make it
and commit the CL - which I did while you were writing.

[tkent, 2010-02-02 09:23:38]

> Sorry for not being clearer: It was such a minor change, so would just make it
> and commit the CL - which I did while you were writing.

Oh, thanks :-)