Apart from the assumption of sequentiality, LGTM w/ comments.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc File src/json-parser.cc (right): http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode81 src/json-parser.cc:81: message = "unexpected_token_string"; This might change the behavior for unterminated strings, but I guess that's acceptable. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode83 src/json-parser.cc:83: default: Agree, it's probably better to not have the unexpected_token_identifier message for JSON. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode84 src/json-parser.cc:84: message = "unexpected_token"; The unexpected_token message needs/expects second argument. See messages.js. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode121 src/json-parser.cc:121: AdvanceWS(); Don't use AdvanceWS here, just check for non-identifier-part of c0_ after matching 'e', and let the main scan loop skip whitespace. Make sure that the cursor position is correct (pointing to the 'f') when reporting the incorrect identifier. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode124 src/json-parser.cc:124: return ReportUnexpectedToken(); Maybe change the name, now that we don't use Token values. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode129 src/json-parser.cc:129: AdvanceWS(); As above. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode137 src/json-parser.cc:137: AdvanceWS(); And again. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode176 src/json-parser.cc:176: } while (c0_ == ',' && AdvanceWS()); Ah, so that's why AdvanceWS returns true. Don't do that! In general, don't have side effects in condition expressions. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode186 src/json-parser.cc:186: Handle<Object> JsonParser::ParseJsonArray() { If position must be at '[', do ASSERT_EQ(c0_, '['); http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h File src/json-parser.h (right): http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h#newcode53 src/json-parser.h:53: } else if (is_sequential_ascii_) { This was changed? http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h#newcode64 src/json-parser.h:64: inline bool AdvanceWS() { Don't abbreviate unless necessary. Call this AdvanceSkipWhitespace. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h#newcode68 src/json-parser.h:68: return true; Why the return true? If it always returns the same value, it should be superfluous. http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h#newcode71 src/json-parser.h:71: inline void SkipWS() { And just SkipWhitespace. http://codereview.chromium.org/7020018/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
