Issue 7020018: Remove scanner abstraction layer from JSON parsing. The change in api.cc is t...

Issue 7020018: Remove scanner abstraction layer from JSON parsing. The change in api.cc is t... (Closed)

Created:
9 years, 6 months ago by sandholm

Modified:
9 years, 6 months ago

Reviewers:
Lasse Reichstein, Rico

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

Remove scanner abstraction layer from JSON parsing.

Patch Set 6 : '' #

Created: 9 years, 6 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+172 lines, -269 lines)			Patch
	M	src/json-parser.h	View	1 2 3 4 5	4 chunks	+48 lines, -43 lines	0 comments	Download
	M	src/json-parser.cc	View	1 2 3 4 5	11 chunks	+124 lines, -226 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Lasse Reichstein

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: ...

9 years, 6 months ago (2011-06-01 11:03:58 UTC) #2

sandholm

9 years, 6 months ago (2011-06-01 13:45:20 UTC) #3

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.

Expand Messages | Collapse Messages