Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096002:
StringToDouble rewritten not using String::Get and memory allocations.... (Closed)
|
Created:
10 years, 9 months ago by SeRya Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionStringToDouble rewritten not using String::Get and memory allocations.
It converts the number to "canonical" form removing insignificant digits,
leading zerroes and spaces what guarantees to fit a fixed size buffer and
does not changes result of strtod.
Committed: http://code.google.com/p/v8/source/detail?r=4241
Patch Set 1 #Patch Set 2 : '' #
Total comments: 52
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 12
Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 17
Messages
Total messages: 9 (0 generated)
StringToNumber is called ~10M times in SunSpider (according to my measurements) but vast majority of arguments are junk values. Tests that heavy convert numbers spend up to 20% their time allocating/deallocating memory in the C heap. In addition to this change I'm going to add a fast check for obviously junk string (I'm deciding if it should be added it into the Runtime_NumberToString to inlined).
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/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 ?
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
I will discuss the 100000000000000000000000.0000000000000000000000000000000000000000000001 issue with the V8 team on monday. The way I see it we have two options: 1. Follow ECMA-262 and round down, thus being incompatible with older versions. 2. Fallback to a more expensive reading when there are more than 20 digits. Pros/Cons for 1: Pro: Basically nothing to do. That's what we have now. Cons: Incompatible and we might numbers the "wrong" way. On the other hand these numbers have to be written by hand (toString/toExponential/toFixed will never produce a number that would make such problems). Therefore they are extremely rare. Pros/Cons for 2: Pro: Compatible with older variants of V8. Reading is correct. Might slightly simplify the fast case: the exponent would need to be in range -999 to 999. Cons: we would need to keep/add a fallback method. Maybe a template taking either a fixed-size buffer or a dynamic vector would do the trick, though. 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/19 15:46:12, SeRya wrote: > 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). I'm with Erik here. I still don't understand how this actually types. (Although I'm by no means a C++ expert). Also operator-overloading should be rare in Google code. Why not Peek(), AtEnd(), etc? This said, I'm not very familiar with V8 coding practices. http://codereview.chromium.org/1096002/diff/3002/4003#newcode496 src/conversions.cc:496: const int max_exponent = INT_MAX / 2; On 2010/03/19 15:46:12, SeRya wrote: > 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. Right you are. http://codereview.chromium.org/1096002/diff/3002/4003#newcode519 src/conversions.cc:519: if (exponent != 0) { On 2010/03/19 15:46:12, SeRya wrote: > 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. my comment was based on the assumption that the read exponent was in range -400 to +400. So disregard it. http://codereview.chromium.org/1096002/diff/21004/27003#newcode298 src/conversions.cc:298: // 1. currnet == end (other ops are not allowed), current != end. Are we sure there is at least one character? If yes assert it. If not, and it is legal to access current[0] of empty string, explain. http://codereview.chromium.org/1096002/diff/21004/27003#newcode330 src/conversions.cc:330: buffer[buffer_pos++] = '-'; It might make sense to move the hexadecimal reading into a separate function. http://codereview.chromium.org/1096002/diff/21004/27003#newcode405 src/conversions.cc:405: if (current == end) return signed_zero; I think it makes more sense to structure as follows: if (current == end) { if (significant_digits == 0 && !leading_zero) { // String was ".". return JUNK_STRING_VALUE; } else { goto parsing_done; } } if (significant_digits == 0) { octal = false; ... } http://codereview.chromium.org/1096002/diff/21004/27003#newcode451 src/conversions.cc:451: } How should "123e" be parsed when "trailing junk is enabled? as "123" or as JUNK_STRING_VALUE? If it's the latter, then this is fine. http://codereview.chromium.org/1096002/diff/21004/27003#newcode456 src/conversions.cc:456: ++current; As before: should 123e+ be parsed as 123 or JUNK_STRING_VALUE when trailing junk is enabled.
LGTM if you increase the buffer size to the max-double string. Please upload a new version here before committing (just to make comparison easier). http://codereview.chromium.org/1096002/diff/21004/27003 File src/conversions.cc (right): http://codereview.chromium.org/1096002/diff/21004/27003#newcode123 src/conversions.cc:123: int StringInputBufferIterator::operator * () const { Convention v8 seems to be to put the attach the operator: operator*()
Buffer size enlarged to 772. Added code checking if all of the dropped digits are zeros and appenging '1' (or '.1') if not. Added a test for 1000-digit number. http://codereview.chromium.org/1096002/diff/21004/27003 File src/conversions.cc (right): http://codereview.chromium.org/1096002/diff/21004/27003#newcode123 src/conversions.cc:123: int StringInputBufferIterator::operator * () const { On 2010/03/23 12:34:54, Florian Loitsch wrote: > Convention v8 seems to be to put the attach the operator: operator*() Done. http://codereview.chromium.org/1096002/diff/21004/27003#newcode298 src/conversions.cc:298: if (*current == '+') { On 2010/03/20 16:56:51, Florian Loitsch wrote: > Are we sure there is at least one character? > If yes assert it. > If not, and it is legal to access current[0] of empty string, explain. If string is empty this line wouldn't be reached (because of 'if (!AdvanceToNonspace(¤t, end)) return empty_string_val;'). Added explanation as a comment. http://codereview.chromium.org/1096002/diff/21004/27003#newcode330 src/conversions.cc:330: if ((flags & ALLOW_HEX) && (*current == 'x' || *current == 'X')) { On 2010/03/20 16:56:51, Florian Loitsch wrote: > It might make sense to move the hexadecimal reading into a separate function. Done. http://codereview.chromium.org/1096002/diff/21004/27003#newcode405 src/conversions.cc:405: if (significant_digits == 0) { On 2010/03/20 16:56:51, Florian Loitsch wrote: > I think it makes more sense to structure as follows: > if (current == end) { > if (significant_digits == 0 && !leading_zero) { > // String was ".". > return JUNK_STRING_VALUE; > } else { > goto parsing_done; > } > } > if (significant_digits == 0) { > octal = false; > ... > } Done. http://codereview.chromium.org/1096002/diff/21004/27003#newcode451 src/conversions.cc:451: if (current == end) return JUNK_STRING_VALUE; On 2010/03/20 16:56:51, Florian Loitsch wrote: > How should "123e" be parsed when "trailing junk is enabled? > as "123" or as JUNK_STRING_VALUE? > If it's the latter, then this is fine. Fixed. Test added. http://codereview.chromium.org/1096002/diff/21004/27003#newcode456 src/conversions.cc:456: if (current == end) return JUNK_STRING_VALUE; On 2010/03/20 16:56:51, Florian Loitsch wrote: > As before: should 123e+ be parsed as 123 or JUNK_STRING_VALUE when trailing junk > is enabled. Fixed. Test added.
LGTM http://codereview.chromium.org/1096002/diff/3003/39003 File src/conversions.cc (right): http://codereview.chromium.org/1096002/diff/3003/39003#newcode280 src/conversions.cc:280: // Hexidecomal may have (52) / 4 + 1 significant digit. Mean of 2 Hexidecomal -> Hexadecimal digit -> digits I can't understand the sentence that starts "Mean of 2" http://codereview.chromium.org/1096002/diff/3003/39003#newcode281 src/conversions.cc:281: // hexidecimal may have n + 1. hexidecimal -> hexadecimal http://codereview.chromium.org/1096002/diff/3003/39003#newcode282 src/conversions.cc:282: const int max_significant_digits = (52) / 4 + 2; I don't see the point in putting 52 in brackets here. max_significant_digits should be named as a constant ie kMaxSignificantDigits http://codereview.chromium.org/1096002/diff/3003/39003#newcode328 src/conversions.cc:328: // Multiplying by power of 2 doesn't cause a loss of precision. power -> a power http://codereview.chromium.org/1096002/diff/3003/39003#newcode345 src/conversions.cc:345: // To make sure that iterator unreferencing is valid the following unreferencing -> dereferencing http://codereview.chromium.org/1096002/diff/3003/39003#newcode351 src/conversions.cc:351: // 4. 'current' is not unreferenced after the 'parsing_done' label. and here http://codereview.chromium.org/1096002/diff/3003/39003#newcode358 src/conversions.cc:358: const int max_significant_digits = 772; Name as a constant. Is it possible to provide a one or two line explanation for why this is enough? http://codereview.chromium.org/1096002/diff/3003/39003#newcode360 src/conversions.cc:360: const int buffer_size = max_significant_digits + 10; Name as a constant. http://codereview.chromium.org/1096002/diff/3003/39003#newcode374 src/conversions.cc:374: // Ignore leading sign; Skip following spaces. Skip -> skip http://codereview.chromium.org/1096002/diff/3003/39003#newcode384 src/conversions.cc:384: static const char infinity_symbol[] = "Infinity"; Name as a constant. http://codereview.chromium.org/1096002/diff/3003/39003#newcode409 src/conversions.cc:409: double result = InternalHexidecimalStringToDouble(current, Hexi -> Hexa http://codereview.chromium.org/1096002/diff/3003/39003#newcode413 src/conversions.cc:413: return (buffer_pos > 0 && buffer[0] == '-') ? -result : result; Should this be buffer[buffer_pos - 1]? http://codereview.chromium.org/1096002/diff/3003/39003#newcode546 src/conversions.cc:546: // ALLOW_OCTALS has set and there is no '8' and '9' in insignificant '8' and '9' -> '8' or '9'
LGTM. http://codereview.chromium.org/1096002/diff/3003/39003 File src/conversions.cc (right): http://codereview.chromium.org/1096002/diff/3003/39003#newcode280 src/conversions.cc:280: // Hexidecomal may have (52) / 4 + 1 significant digit. Mean of 2 On 2010/03/25 12:26:16, Erik Corry wrote: > Hexidecomal -> Hexadecimal > digit -> digits > I can't understand the sentence that starts "Mean of 2" Maybe the following explanation is easier to understand: A double has a 53bit significand (once the hidden bit has been added). Halfway cases thus have at most 54bits. Therefore 54/4 + 1 digits are sufficient to represent halfway cases. By adding another digit we can keep track of dropped digits. This explanation is still not perfect, but probably better than the current comment. http://codereview.chromium.org/1096002/diff/3003/39003#newcode413 src/conversions.cc:413: return (buffer_pos > 0 && buffer[0] == '-') ? -result : result; On 2010/03/25 12:26:16, Erik Corry wrote: > Should this be buffer[buffer_pos - 1]? lgtm. http://codereview.chromium.org/1096002/diff/3003/39003#newcode546 src/conversions.cc:546: // ALLOW_OCTALS has set and there is no '8' and '9' in insignificant ALLOW_OCTALS is set ... http://codereview.chromium.org/1096002/diff/3003/39001 File test/cctest/test-conversions.cc (right): http://codereview.chromium.org/1096002/diff/3003/39001#newcode154 test/cctest/test-conversions.cc:154: int n = snprintf(buffer, sizeof(buffer), "%.1000Le", (a + b) / 2); It is (imho) better not to rely on snprintf. Precomputing the string and pasting it here will use up more than 10 lines, but at least there would be no dependency on sprintf. Also it is not certain that sprintf behaves correctly on all platforms (for instance on embedded devices). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
