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";
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
This might change the behavior for unterminated strings, but I guess
that's
acceptable.
Done.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode83
src/json-parser.cc:83: default:
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
Agree, it's probably better to not have the
unexpected_token_identifier message
for JSON.
Done.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode84
src/json-parser.cc:84: message = "unexpected_token";
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
The unexpected_token message needs/expects second argument. See
messages.js.
Done.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode121
src/json-parser.cc:121: AdvanceWS();
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
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.
I think this is fine. The invariant is that we're at the next
non-whitespace char. Also, I think it's fine to use whatever position
we're when failing. E.g. with "falsse" we would report the error here:
fals(s)e.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode124
src/json-parser.cc:124: return ReportUnexpectedToken();
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
Maybe change the name, now that we don't use Token values.
Done.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode129
src/json-parser.cc:129: AdvanceWS();
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
As above.
ditto
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode137
src/json-parser.cc:137: AdvanceWS();
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
And again.
ditto
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode176
src/json-parser.cc:176: } while (c0_ == ',' && AdvanceWS());
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
Ah, so that's why AdvanceWS returns true.
Don't do that! In general, don't have side effects in condition
expressions.
Fixed with a AdvanceWhiteSpacesOnlyIfMatch(',') call.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.cc#newcode186
src/json-parser.cc:186: Handle<Object> JsonParser::ParseJsonArray() {
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
If position must be at '[', do
ASSERT_EQ(c0_, '[');
Done.
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_) {
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
This was changed?
r8142 ensures this was not changed.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h#newcode64
src/json-parser.h:64: inline bool AdvanceWS() {
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
Don't abbreviate unless necessary.
Call this AdvanceSkipWhitespace.
Done.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h#newcode68
src/json-parser.h:68: return true;
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
Why the return true? If it always returns the same value, it should be
superfluous.
Done.
http://codereview.chromium.org/7020018/diff/2003/src/json-parser.h#newcode71
src/json-parser.h:71: inline void SkipWS() {
On 2011/06/01 11:03:58, Lasse Reichstein wrote:
And just SkipWhitespace.
Done.
http://codereview.chromium.org/7020018/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
