StringToInt rewritten. This version doesn't allocate memory for long decimals...

StringToInt rewritten. This version doesn't allocate memory for long decimals and uses percise rounding if radix 10 or a power of 2 (in other cases rounding error still may occur). Handling special values moved from Runtime_StringParseInt into StringToInt in order to make it consistent with StringToDouble.


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

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

[Florian Loitsch, 2010-03-30 15:08:43]

LGTM with comments.

http://codereview.chromium.org/1529004/diff/8001/9001
File src/conversions.cc (right):

http://codereview.chromium.org/1529004/diff/8001/9001#newcode246
src/conversions.cc:246: static double InternalStringToInt(Iterator current,
indentation seems unnecessary.

http://codereview.chromium.org/1529004/diff/8001/9001#newcode276
src/conversions.cc:276: if (current == end) return SignedZero(sign);
0x and 0X should evaluate to 0 or NaN depending on who called StringToInt.
"0x    " - 3      => -3
parseInt("0x   ") => NaN

http://codereview.chromium.org/1529004/diff/8001/9001#newcode290
src/conversions.cc:290: if (current == end) return SignedZero(sign);
same here.

http://codereview.chromium.org/1529004/diff/8001/9001#newcode312
src/conversions.cc:312: if ((radix & (radix - 1)) == 0) {
use IsPowerOf2 (see utils.h)

http://codereview.chromium.org/1529004/diff/8001/9001#newcode340
src/conversions.cc:340: const int kBufferSize = kMaxSignificantDigits + 2;
Explain why +2.

http://codereview.chromium.org/1529004/diff/8001/9002
File src/runtime.cc (right):

http://codereview.chromium.org/1529004/diff/8001/9002#newcode4683
src/runtime.cc:4683: return Heap::NumberFromDouble(value);
leave RUNTIME_ASSERT(radix == 0 || 2<= radix && radix <= 36);

[SeRya, 2010-03-31 09:34:58]

http://codereview.chromium.org/1529004/diff/8001/9001
File src/conversions.cc (right):

http://codereview.chromium.org/1529004/diff/8001/9001#newcode246
src/conversions.cc:246: static double InternalStringToInt(Iterator current,
On 2010/03/30 15:08:44, Florian Loitsch wrote:
> indentation seems unnecessary.

Done.

http://codereview.chromium.org/1529004/diff/8001/9001#newcode276
src/conversions.cc:276: if (current == end) return SignedZero(sign);
On 2010/03/30 15:08:44, Florian Loitsch wrote:
> 0x and 0X should evaluate to 0 or NaN depending on who called StringToInt.
> "0x    " - 3      => -3
> parseInt("0x   ") => NaN
> 

Fixed. Test added.

http://codereview.chromium.org/1529004/diff/8001/9001#newcode290
src/conversions.cc:290: if (current == end) return SignedZero(sign);
On 2010/03/30 15:08:44, Florian Loitsch wrote:
> same here.

Fixed. Test added.

http://codereview.chromium.org/1529004/diff/8001/9001#newcode312
src/conversions.cc:312: if ((radix & (radix - 1)) == 0) {
On 2010/03/30 15:08:44, Florian Loitsch wrote:
> use IsPowerOf2 (see utils.h)

Done.

http://codereview.chromium.org/1529004/diff/8001/9001#newcode340
src/conversions.cc:340: const int kBufferSize = kMaxSignificantDigits + 2;
On 2010/03/30 15:08:44, Florian Loitsch wrote:
> Explain why +2.

Done.

http://codereview.chromium.org/1529004/diff/8001/9002
File src/runtime.cc (right):

http://codereview.chromium.org/1529004/diff/8001/9002#newcode4683
src/runtime.cc:4683: return Heap::NumberFromDouble(value);
On 2010/03/30 15:08:44, Florian Loitsch wrote:
> leave RUNTIME_ASSERT(radix == 0 || 2<= radix && radix <= 36);

Done.

[SeRya, 2010-03-31 14:17:10]

Fixed issue with long decimal numbers what failed webkit tests. Fixed Number('0x
'). Tests added.

[Florian Loitsch, 2010-03-31 16:49:59]

LGTM.

http://codereview.chromium.org/1529004/diff/29001/30003
File src/conversions.cc (right):

http://codereview.chromium.org/1529004/diff/29001/30003#newcode335
src/conversions.cc:335: // The buffer may contain up to kMaxSignificantDigits +
1 digits and a zero
Maybe add comment:
A number with more than kMaxSignificantDigits digits will be read as Infinity.

[SeRya, 2010-03-31 17:18:08]

http://codereview.chromium.org/1529004/diff/29001/30003
File src/conversions.cc (right):

http://codereview.chromium.org/1529004/diff/29001/30003#newcode335
src/conversions.cc:335: // The buffer may contain up to kMaxSignificantDigits +
1 digits and a zero
On 2010/03/31 16:50:00, Florian Loitsch wrote:
> Maybe add comment:
> A number with more than kMaxSignificantDigits digits will be read as Infinity.

The comment 7 lines below says the same. I think that is place more suitable
since digits are dropped there and it explaines why it's safe.

[Florian Loitsch, 2010-04-03 13:10:58]

LGTM.

http://codereview.chromium.org/1529004/diff/29001/30003
File src/conversions.cc (right):

http://codereview.chromium.org/1529004/diff/29001/30003#newcode335
src/conversions.cc:335: // The buffer may contain up to kMaxSignificantDigits +
1 digits and a zero
On 2010/03/31 17:18:08, SeRya wrote:
> On 2010/03/31 16:50:00, Florian Loitsch wrote:
> > Maybe add comment:
> > A number with more than kMaxSignificantDigits digits will be read as
Infinity.
> 
> The comment 7 lines below says the same. I think that is place more suitable
> since digits are dropped there and it explaines why it's safe.
true. fine with me.