LGTM
http://codereview.chromium.org/1096002/diff/3002/4003
File src/conversions.cc (right):
http://codereview.chromium.org/1096002/diff/3002/4003#newcode109
src/conversions.cc:109: bool operator != (EndMarker const& m) const {
return !(*this == m); }
Some funky C++ here :-). return !end_; seems simpler, but perhaps this
is somehow better?
http://codereview.chromium.org/1096002/diff/3002/4003#newcode298
src/conversions.cc:298: // 1. currnet == end (other ops are not
allowed), current != end.
currnet -> current
http://codereview.chromium.org/1096002/diff/3002/4003#newcode300
src/conversions.cc:300: // 3. ++currenet (advances the position).
currenet -> current
http://codereview.chromium.org/1096002/diff/3002/4003#newcode308
src/conversions.cc:308: const bool allow_tailing_junk = (flags &
ALLOW_TRAILING_JUNK) != 0;
tailing -> trailing
http://codereview.chromium.org/1096002/diff/3002/4003#newcode310
src/conversions.cc:310: // Insignificant digits will be removed.
This seems suspect to me. How do you know how many digits are
significant for decimals?
http://codereview.chromium.org/1096002/diff/3002/4003#newcode318
src/conversions.cc:318: // or insignificant leading zerroes of the
fractional part are dropped.
zerroes -> zeros
http://codereview.chromium.org/1096002/diff/3002/4003#newcode349
src/conversions.cc:349: bool leading_zerro = false;
zerro -> zero
http://codereview.chromium.org/1096002/diff/3002/4003#newcode356
src/conversions.cc:356: // It could be heximal value.
heximal -> hexadecimal
http://codereview.chromium.org/1096002/diff/3002/4003#newcode374
src/conversions.cc:374: buffer[buffer_pos++] = *current;
Places like this you should assert that you didn't overflow the buffer.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode396
src/conversions.cc:396: // Multiplying by power of 2 doesn't cause
loosing percision.
loosing percision -> a loss of precision
(and loosing should have been losing)
http://codereview.chromium.org/1096002/diff/3002/4003#newcode402
src/conversions.cc:402: // Ignore leading zerroes in the integer part.
zerroes -> zeros and in subsequent comments.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode416
src/conversions.cc:416: // Integer part consists of 0 or absends.
Significant digits start after
absends?
http://codereview.chromium.org/1096002/diff/3002/4003#newcode474
src/conversions.cc:474: // If there is leading true then string was
[+-]0+
This comment is hard to parse.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode476
src/conversions.cc:476: // If significant_digits != 0 the string does
not equal to 0.
does not equal to -> is not equal to
http://codereview.chromium.org/1096002/diff/3002/4003#newcode477
src/conversions.cc:477: // Otherwize there is no digits in the string.
is -> are
Otherwize -> Otherwise
http://codereview.chromium.org/1096002/diff/3002/4003#newcode492
src/conversions.cc:492: if (current == end && *current < '0' && *current
'9') {
Are you dereferencing current after you have ascertained that you are at
the end?
http://codereview.chromium.org/1096002/diff/3002/4001
File test/cctest/test-conversions.cc (right):
http://codereview.chromium.org/1096002/diff/3002/4001#newcode101
test/cctest/test-conversions.cc:101: CHECK_EQ(-1.0, StringToDouble(" -
1 ", NO_FLAGS));
Your code also allows for spaces after a unary +, but you don't seem to
test that.
I presume that we match jsc on these tests.
http://codereview.chromium.org/1096002/diff/3002/4002
File test/mjsunit/str-to-num.js (right):
http://codereview.chromium.org/1096002/diff/3002/4002#newcode138
test/mjsunit/str-to-num.js:138: assertEquals(15, toNumber("0x00F"));
We need some tests of huge hex and octal numbers that overflow the max
expected number of digits.
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.
