http://codereview.chromium.org/1096002/diff/3002/4003 File src/conversions.cc (right):
http://codereview.chromium.org/1096002/diff/3002/4003#newcode162 src/conversions.cc:162: if (*s == '-' || *s == '+') s++; Not really important: compilers are usually better with array indices than with pointer-arithmetic. I would have considered using and index variable instead of changing the pointer. http://codereview.chromium.org/1096002/diff/3002/4003#newcode310 src/conversions.cc:310: // Insignificant digits will be removed. On 2010/03/18 20:34:22, Erik Corry wrote:
This seems suspect to me. How do you know how many digits are
significant for
decimals?
I just looked at the specification again. Ecma-262 (old and new version) allow to cut off after the 20th digit (and to round up or down). In theory Erik is right, though. Try the following number: 100000000000000000000000.0000000000000000000000000000000000000000000001 If you remove the last '1' you will get a different number. Even worse: this can be continued indefinitely. I don't know if V8 could switch implementation since this would probably break compatibility with other implementations. On the other hand this should not happen often. http://codereview.chromium.org/1096002/diff/3002/4003#newcode408 src/conversions.cc:408: Avoid duplicating the code for reading the fractional digits. I would prefer seeing code like this (no need to put it into separate methods since that would make exits more difficult): ReadLeadingZeros(); ReadIntegralDigits(); if (dot) { if (no_digit_yet) { ReadLeadingZeros(); } ReadFractionalDigits(); } ReadExponent() http://codereview.chromium.org/1096002/diff/3002/4003#newcode438 src/conversions.cc:438: // Copy signinficant digits of the integer part (if any) to the buffer. significant http://codereview.chromium.org/1096002/diff/3002/4003#newcode496 src/conversions.cc:496: const int max_exponent = INT_MAX / 2; This seems to be too complicated. A decimal number without leading 0s may only have a decimal exponent of ~-400 to ~+400 before ending up being infinite or 0. http://codereview.chromium.org/1096002/diff/3002/4003#newcode506 src/conversions.cc:506: num = num * 10 + digit; if num == max_exponent, then multiplying it by 10 will overflow. This should be in the 'else' part of the 'if'. http://codereview.chromium.org/1096002/diff/3002/4003#newcode519 src/conversions.cc:519: if (exponent != 0) { not that it really matters, but you could copy the exponent characters while reading them, and just stop after 4 digits. This way you could avoid this part here. http://codereview.chromium.org/1096002/diff/3002/4003#newcode564 src/conversions.cc:564: } else if (shape.IsSequentialAscii()) { Shouldn't this be shape.IsSequentialTwoByte ? http://codereview.chromium.org/1096002 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
