Please have another look
http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc File src/json-parser.cc (right): http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode46 src/json-parser.cc:46: is_sequential_ascii_ = true; On 2011/05/19 07:27:40, Lasse Reichstein wrote:
This is not a property that is guaranteed to be preserved over time. A
string
can change from being ASCII to being two-byte due to it being used in
the DOM
(because WebKit is uc16 clean internally).
I.e., you have to check again every time the code might have been
interrupted. As discussed offline, this is not the case right? http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode145 src/json-parser.cc:145: Handle<String> key = GetString(true); On 2011/05/19 07:27:40, Lasse Reichstein wrote:
How about having two functions: GetString() and GetSymbol(), that both
call a
utility function with true/false (or something). Makes it easier to
read, since
you don't have to know what "true" means here. (i.e., "no magic numbers" should extend to "no magic booleans").
Done. http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode344 src/json-parser.cc:344: } else { On 2011/05/19 07:27:40, Lasse Reichstein wrote:
Ick. Why create a heap string? Just make a buffer and do WriteToFlat of the number (you have start
and end pos)
to the buffer, and parse from that using the fast atoi.
Fast atoi? this is a double. I added a conversion method taking a uc16 vector and are using this instead http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode363 src/json-parser.cc:363: ASSERT(two_byte->IsSeqTwoByteString()); On 2011/05/19 07:27:40, Lasse Reichstein wrote:
Odd assert. How could that not happen? Consider changing the return type of Factory::NewRawTwoByteString to SeqTwoByteString.
Done. http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode439 src/json-parser.cc:439: count); On 2011/05/19 07:27:40, Lasse Reichstein wrote:
If the ascii string is short (e.g., it's shorter than a cons string),
you might
want to copy the ascii characters to the two-byte string instead of
creating a
cons.
You might even want to not create the ASCII string first in that case.
Indeed, this is one of those optimizations for the second round ;-) http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode458 src/json-parser.cc:458: return SlowScanJsonString(); On 2011/05/19 07:27:40, Lasse Reichstein wrote:
The SlowScanJsonString creates a TwoByte string. Isn't that overkill
for
something containing escapes that are still ASCII (e.g., the very
plausible \n,
\r, \t,\\ or \").
Yes http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode470 src/json-parser.cc:470: if (!string_val_.is_null()) return string_val_; On 2011/05/19 07:27:40, Lasse Reichstein wrote:
That does not make it a symbol if it isn't already.
renaming hint_symbol http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode481 src/json-parser.cc:481: return isolate()->factory()->NewSubString( On 2011/05/19 07:27:40, Lasse Reichstein wrote:
This also doesn't make it a symbol even if is_symbol is true. If this function is not guaranteed to return a symbol when is_symbol
is true,
rename it to "hint_symbol" (or something literal).
Done. http://codereview.chromium.org/7039037/diff/1/src/json-parser.h File src/json-parser.h (right): http://codereview.chromium.org/7039037/diff/1/src/json-parser.h#newcode46 src/json-parser.h:46: JsonParser() : isolate_(Isolate::Current()) {} On 2011/05/19 07:27:40, Lasse Reichstein wrote:
Get the isolate from the input string instead.
Done. http://codereview.chromium.org/7039037/diff/1/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/7039037/diff/1/src/objects.cc#newcode5478 src/objects.cc:5478: if (Get(i) != static_cast<uint16_t>(str[i])) return false; On 2011/05/19 07:27:40, Lasse Reichstein wrote:
Potentially unnecessarily slow for deeply nested cons strings. Consider doing a recursive version that traverses only once, and
checks
subvectors agains substrings.
This is the existing version, so I will leave as is for now http://codereview.chromium.org/7039037/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
