I'm not sure what to do about
100000000000000000000000.0000000000000000000000000000000000000000000001 (by
the
way, Firefox also spots removing the last 1). Does it stick us to
implementation
which require memory allocation?
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); }
On 2010/03/18 20:34:22, Erik Corry wrote:
Some funky C++ here :-). return !end_; seems simpler, but perhaps
this is
somehow better?
Just a canonical form of != which simplifies maintenance (IMHO).
http://codereview.chromium.org/1096002/diff/3002/4003#newcode298
src/conversions.cc:298: // 1. currnet == end (other ops are not
allowed), current != end.
On 2010/03/18 20:34:22, Erik Corry wrote:
currnet -> current
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode300
src/conversions.cc:300: // 3. ++currenet (advances the position).
On 2010/03/18 20:34:22, Erik Corry wrote:
currenet -> current
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode308
src/conversions.cc:308: const bool allow_tailing_junk = (flags &
ALLOW_TRAILING_JUNK) != 0;
On 2010/03/18 20:34:22, Erik Corry wrote:
tailing -> trailing
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode318
src/conversions.cc:318: // or insignificant leading zerroes of the
fractional part are dropped.
On 2010/03/18 20:34:22, Erik Corry wrote:
zerroes -> zeros
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode349
src/conversions.cc:349: bool leading_zerro = false;
On 2010/03/18 20:34:22, Erik Corry wrote:
zerro -> zero
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode356
src/conversions.cc:356: // It could be heximal value.
On 2010/03/18 20:34:22, Erik Corry wrote:
heximal -> hexadecimal
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode374
src/conversions.cc:374: buffer[buffer_pos++] = *current;
On 2010/03/18 20:34:22, Erik Corry wrote:
Places like this you should assert that you didn't overflow the
buffer.
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode396
src/conversions.cc:396: // Multiplying by power of 2 doesn't cause
loosing percision.
On 2010/03/18 20:34:22, Erik Corry wrote:
loosing percision -> a loss of precision
(and loosing should have been losing)
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode402
src/conversions.cc:402: // Ignore leading zerroes in the integer part.
On 2010/03/18 20:34:22, Erik Corry wrote:
zerroes -> zeros and in subsequent comments.
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode408
src/conversions.cc:408:
On 2010/03/19 13:39:43, Florian Loitsch wrote:
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()
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode416
src/conversions.cc:416: // Integer part consists of 0 or absends.
Significant digits start after
On 2010/03/18 20:34:22, Erik Corry wrote:
absends?
Done.
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.
On 2010/03/19 13:39:43, Florian Loitsch wrote:
significant
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode474
src/conversions.cc:474: // If there is leading true then string was
[+-]0+
On 2010/03/18 20:34:22, Erik Corry wrote:
This comment is hard to parse.
Fixed.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode476
src/conversions.cc:476: // If significant_digits != 0 the string does
not equal to 0.
On 2010/03/18 20:34:22, Erik Corry wrote:
does not equal to -> is not equal to
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode477
src/conversions.cc:477: // Otherwize there is no digits in the string.
On 2010/03/18 20:34:22, Erik Corry wrote:
is -> are
Otherwize -> Otherwise
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode492
src/conversions.cc:492: if (current == end && *current < '0' && *current
'9') {
On 2010/03/18 20:34:22, Erik Corry wrote:
Are you dereferencing current after you have ascertained that you are
at the
end?
It's a bug. I added a test.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode496
src/conversions.cc:496: const int max_exponent = INT_MAX / 2;
On 2010/03/19 13:39:43, Florian Loitsch wrote:
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.
1<1000 zeros>e-1000 == 1.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode506
src/conversions.cc:506: num = num * 10 + digit;
On 2010/03/19 13:39:43, Florian Loitsch wrote:
if num == max_exponent, then multiplying it by 10 will overflow. This
should be
in the 'else' part of the 'if'.
Done.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode519
src/conversions.cc:519: if (exponent != 0) {
On 2010/03/19 13:39:43, Florian Loitsch wrote:
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.
It would mean another chunk of code that which drop leading zeros and
check for junk tail. I'd prefer to simplify for now and may be add this
optimization later.
http://codereview.chromium.org/1096002/diff/3002/4003#newcode564
src/conversions.cc:564: } else if (shape.IsSequentialAscii()) {
On 2010/03/19 13:39:43, Florian Loitsch wrote:
Shouldn't this be shape.IsSequentialTwoByte ?
It must. Thank you for finding the bug. It prevents from using
optimization for sequential unicode strings.
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));
On 2010/03/18 20:34:22, Erik Corry wrote:
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.
Done.
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"));
On 2010/03/18 20:34:22, Erik Corry wrote:
We need some tests of huge hex and octal numbers that overflow the max
expected
number of digits.
Added tests for huge hex here, for octals into test-conversions.cc
http://codereview.chromium.org/1096002
--
v8-dev mailing list
v8-dev@googlegroups.com
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.
