Percise rounding parsing octal and hexadecimal strings....

Percise rounding parsing octal and hexadecimal strings.

Rounding happens when the number exceeds 53 bits of floating point mantissa. Current implemetation ignores digits after some limits. 0x1000000000000081 was rounded to 0x1000000000000100 while 0x100000000000008000001 was rounded to 0x100000000000000000000.


Committed: http://code.google.com/p/v8/source/detail?r=4309

[Florian Loitsch, 2010-03-29 11:12:23]

LGTM with comments.
Are the existing unit-tests covering all the cases?

http://codereview.chromium.org/1374005/diff/6001/7003
File src/conversions.cc (right):

http://codereview.chromium.org/1374005/diff/6001/7003#newcode277
src/conversions.cc:277: static bool inline isDigit(int x, int radix) {
I don't think it's necessary to mark a function as 'inline'. The compiler is
usually better at these things than we are.
ymmv

http://codereview.chromium.org/1374005/diff/6001/7003#newcode285
src/conversions.cc:285: template <int radix_log_2, class Iterator, class
EndMark>
Add comment that current must not be end.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode318
src/conversions.cc:318: number = (number << radix_log_2) | digit;
number = number * radix + digit.
The compiler will be smart enough to change that into a shifts and pluses.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode335
src/conversions.cc:335: if (dropped_bits >= middle_value) {
Since half-way cases do not always round down (see my next comment) I suggest
splitting into three cases:
if (dropped_bits < middle_value) {
  // round down
} else if (dropped_bits > middle_value) {
  // round up
} else {  // dropped_bits == middle_value
  // wait for next digits
}

http://codereview.chromium.org/1374005/diff/6001/7003#newcode338
src/conversions.cc:338: // if the tail consists of zeros.
I don't think this is the right approach. Generally we prefer rounding towards
even. That is, if the last non-dropped digit is even, round down, otherwise
round up.
Otherwise we obtain different numbers when reading a number as decimal or as
hexadecimal.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode339
src/conversions.cc:339: do {
Since we need to run through the remaining digits anyways, it would probably
make sense to determine the final rounding direction in the next loop.
The 'if'-case would be smaller, and I don't think it would add too much in the
next loop.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode352
src/conversions.cc:352: } else {  // driopped_bit > middle_value
dropped

http://codereview.chromium.org/1374005/diff/6001/7003#newcode396
src/conversions.cc:396: // and sign. Assuming it's a rare case more simple code
is used.
You could use/extend double.h. It already contains the reading part. I don't
think adding the writing-part would be difficult.
I expect 'pow' to be optimized for '2', and the given approach is fine.
(Otherwise someone would need to deal with infinity...)

http://codereview.chromium.org/1374005/diff/6001/7003#newcode397
src/conversions.cc:397: printf("Exponent: %d (%f))\n", exponent, pow(2.0,
exponent));
remove printf.

[Florian Loitsch, 2010-03-29 11:20:57]

test-files LGTM.
somehow skipped over the test-file changes. So disregard my last comment about
the test-cases (in the previous mail).

http://codereview.chromium.org/1374005/diff/6001/7002
File test/mjsunit/str-to-num.js (right):

http://codereview.chromium.org/1374005/diff/6001/7002#newcode1
test/mjsunit/str-to-num.js:1: 
No need for new line.

[SeRya, 2010-03-29 14:59:22]

http://codereview.chromium.org/1374005/diff/6001/7003
File src/conversions.cc (right):

http://codereview.chromium.org/1374005/diff/6001/7003#newcode277
src/conversions.cc:277: static bool inline isDigit(int x, int radix) {
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> I don't think it's necessary to mark a function as 'inline'. The compiler is
> usually better at these things than we are.
> ymmv

Done.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode285
src/conversions.cc:285: template <int radix_log_2, class Iterator, class
EndMark>
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> Add comment that current must not be end.

Done.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode318
src/conversions.cc:318: number = (number << radix_log_2) | digit;
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> number = number * radix + digit.
> The compiler will be smart enough to change that into a shifts and pluses.

Done.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode335
src/conversions.cc:335: if (dropped_bits >= middle_value) {
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> Since half-way cases do not always round down (see my next comment) I suggest
> splitting into three cases:
> if (dropped_bits < middle_value) {
>   // round down
> } else if (dropped_bits > middle_value) {
>   // round up
> } else {  // dropped_bits == middle_value
>   // wait for next digits
> }

Done.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode338
src/conversions.cc:338: // if the tail consists of zeros.
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> I don't think this is the right approach. Generally we prefer rounding towards
> even. That is, if the last non-dropped digit is even, round down, otherwise
> round up.
> Otherwise we obtain different numbers when reading a number as decimal or as
> hexadecimal.

OK, I changed code to round half-way numbers (with  insignificant bits
'10...0b') to even.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode339
src/conversions.cc:339: do {
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> Since we need to run through the remaining digits anyways, it would probably
> make sense to determine the final rounding direction in the next loop.
> The 'if'-case would be smaller, and I don't think it would add too much in the
> next loop.
> 

I moved the loop up and added a variable 'zero_tail' to achieve the same effect.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode352
src/conversions.cc:352: } else {  // driopped_bit > middle_value
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> dropped

This line has gone.

http://codereview.chromium.org/1374005/diff/6001/7003#newcode397
src/conversions.cc:397: printf("Exponent: %d (%f))\n", exponent, pow(2.0,
exponent));
On 2010/03/29 11:12:23, Florian Loitsch wrote:
> remove printf.

Done.

http://codereview.chromium.org/1374005/diff/6001/7002
File test/mjsunit/str-to-num.js (right):

http://codereview.chromium.org/1374005/diff/6001/7002#newcode1
test/mjsunit/str-to-num.js:1: 
On 2010/03/29 11:20:57, Florian Loitsch wrote:
> No need for new line.

Done.

[Florian Loitsch, 2010-03-29 15:14:30]

LGTM.

http://codereview.chromium.org/1374005/diff/23001/24003
File src/conversions.cc (right):

http://codereview.chromium.org/1374005/diff/23001/24003#newcode334
src/conversions.cc:334: bool zerro_tail = true;
zero_tail

http://codereview.chromium.org/1374005/diff/23001/24003#newcode350
src/conversions.cc:350: if ((number & 1) != 0 || !zerro_tail) {
Add comment why we look at the last binary digit of the number. (I.e make a
reference to "round to even").

[Florian Loitsch, 2010-03-29 15:14:32]

LGTM.

[SeRya, 2010-03-29 15:36:17]

http://codereview.chromium.org/1374005/diff/23001/24003
File src/conversions.cc (right):

http://codereview.chromium.org/1374005/diff/23001/24003#newcode334
src/conversions.cc:334: bool zerro_tail = true;
On 2010/03/29 15:14:30, Florian Loitsch wrote:
> zero_tail

Done.

http://codereview.chromium.org/1374005/diff/23001/24003#newcode350
src/conversions.cc:350: if ((number & 1) != 0 || !zerro_tail) {
On 2010/03/29 15:14:30, Florian Loitsch wrote:
> Add comment why we look at the last binary digit of the number. (I.e make a
> reference to "round to even").

Done.