Request denied. LGTM.
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; 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. http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode145 src/json-parser.cc:145: Handle<String> key = GetString(true); 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"). http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode344 src/json-parser.cc:344: } else { 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. http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode363 src/json-parser.cc:363: ASSERT(two_byte->IsSeqTwoByteString()); Odd assert. How could that not happen? Consider changing the return type of Factory::NewRawTwoByteString to SeqTwoByteString. http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode439 src/json-parser.cc:439: count); 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. http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode458 src/json-parser.cc:458: return SlowScanJsonString(); 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 \"). 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_; That does not make it a symbol if it isn't already. http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode481 src/json-parser.cc:481: return isolate()->factory()->NewSubString( 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). 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()) {} Get the isolate from the input string instead. 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; Potentially unnecessarily slow for deeply nested cons strings. Consider doing a recursive version that traverses only once, and checks subvectors agains substrings. http://codereview.chromium.org/7039037/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
