LGTM
http://codereview.chromium.org/7241023/diff/3001/src/json-parser.h File src/json-parser.h (right): http://codereview.chromium.org/7241023/diff/3001/src/json-parser.h#newcode111 src/json-parser.h:111: // as first part of a ConsString Comment out of date (what is ascii_count)? http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode113 src/json-parser.h:113: Handle<String> SlowScanJsonTwoByteString(Handle<String> prefix); Slow version of what? Just say what the function does, and only after that maybe say which function is supposed to use it. So it creates a new string, copies prefix[start..end] into the beginning of it (and I assume start can be non-zero when prefix is source_) and starts scanning the string and adding it after the prefix. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode171 src/json-parser.h:171: // Optimized fast case where we only have ascii characters. ASCII is an acronym when used in prose. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode459 src/json-parser.h:459: seq_two_byte->SeqTwoByteStringSet(count++, '\x0d'); Move comment to after "if" line, so it only applies if the condition is true (or make it apply to the check itself, like below: "Check whether we need to create a new string."). Comments should be full sentences (so remember "." at the end). No, we don't always follow that in practice :) http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode466 src/json-parser.h:466: for (int i = 0; i < 4; i++) { Does it lint? (Generally, us kCharSize instead of sizeof(char)). For readability, I'd prefer the operands in the opposite order: if (sizeof(SinkChar) != kCharSize || and you should probably do: if (sizeof(SinkChar) == kUC16Size || since you know that there are only those two possibilities. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode467 src/json-parser.h:467: Advance(); This could use a comment: If the sink can contain UC16 characters, or source_ contains only ASCII characters, there's no need to test whether we can store the character. Otherwise check whether the UC16 source character can fit in the ASCII sink. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode472 src/json-parser.h:472: value = value * 16 + digit; non-ASCII. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode510 src/json-parser.h:510: Handle<SeqAsciiString>::cast(new_ascii); sizeof(SinkChar) == kUC16Size http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode531 src/json-parser.h:531: } Do ASSERT that the string is at the allocation boundary. (It should be, there have been no allocations or possible interruptions since it was allocated). http://codereview.chromium.org/7241023/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
