[parser] unify metaproperty parsing and require unescaped property name

[parser] unify metaproperty parsing and require unescaped property name

BUG=v8:4756
LOG=N
R=adamk@chromium.org, littledan@chromium.org, wingo@igalia.com

Committed: https://crrev.com/fd2edb0ea2b5317c5b27ac3fa40dea9ecf6159f0
Cr-Commit-Position: refs/heads/master@{#34050}

[caitp (gmail), 2016-02-16 20:38:16]

metaproperty thing

[adamk, 2016-02-16 20:52:09]

Nice fix! Please add a case for each to test-parsing.cc to cover the preparser,
and take care of the comments below.

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:457: int ExpectMetaProperty(Vector<const char>
keyword, const char* full,
Some nits on argument names: this should be "property", not "keyword". Also, how
about "full_name" instead of "full"?

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2775: int
ParserBase<Traits>::ExpectMetaProperty(Vector<const char> keyword,
Returning a position isn't something we do elsewhere in the parser, I don't see
a win in introducing a new convention here. Would prefer this be a void function
and keep the position() call outside. You could pass in the position if you want
to avoid redundant position() calls.

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2778: if (*ok) {
I don't think should ever be false, so this if statement isn't needed.

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2785: MessageTemplate::kInvalidEscapedMetaProperty,
full, kSyntaxError);
kSyntaxError is the default

[caitp (gmail), 2016-02-16 22:13:55]

done and added test-parsing.cc tests (which hopefully all pass)

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:457: int ExpectMetaProperty(Vector<const char>
keyword, const char* full,
On 2016/02/16 20:52:09, adamk wrote:
> Some nits on argument names: this should be "property", not "keyword". Also,
how
> about "full_name" instead of "full"?

Done.

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2775: int
ParserBase<Traits>::ExpectMetaProperty(Vector<const char> keyword,
On 2016/02/16 20:52:09, adamk wrote:
> Returning a position isn't something we do elsewhere in the parser, I don't
see
> a win in introducing a new convention here. Would prefer this be a void
function
> and keep the position() call outside. You could pass in the position if you
want
> to avoid redundant position() calls.

Done.

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2778: if (*ok) {
On 2016/02/16 20:52:09, adamk wrote:
> I don't think should ever be false, so this if statement isn't needed.

Done.

https://codereview.chromium.org/1700123003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2785: MessageTemplate::kInvalidEscapedMetaProperty,
full, kSyntaxError);
On 2016/02/16 20:52:09, adamk wrote:
> kSyntaxError is the default

Done.

[adamk, 2016-02-16 22:21:56]

lgtm % compile failure and my guesses at needed test fixes

https://codereview.chromium.org/1700123003/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1700123003/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:2775: int
ParserBase<Traits>::ExpectMetaProperty(Vector<const char> property_name,
Needs to return "void" to make it compile.

https://codereview.chromium.org/1700123003/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1700123003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:8021: };
Think you'll need a NULL here.

https://codereview.chromium.org/1700123003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:8025: i::FLAG_harmony_function_sent = true;
Hmm, I suspect this is supposed to have an allow_* flag for the parser to avoid
threading issues...not something to fix in this patch, obviously.

https://codereview.chromium.org/1700123003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:8040: };
And a NULL here too

https://codereview.chromium.org/1700123003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:8042: RunParserSyncTest(context_data, error_data,
kError, NULL, 0, NULL, 0);
You can leave off the last four arguments in a case like this.

[caitp (gmail), 2016-02-16 22:23:49]

https://codereview.chromium.org/1700123003/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1700123003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:8025: i::FLAG_harmony_function_sent = true;
On 2016/02/16 22:21:55, adamk wrote:
> Hmm, I suspect this is supposed to have an allow_* flag for the parser to
avoid
> threading issues...not something to fix in this patch, obviously.

yeah, I'll add the parser flag accessors in a separate cleanup. Good point about
the missing NULL (I would have built this locally, but just jumped from a
totally different branch, and it takes foreverrrr I really need a better local
build infrastructure :L)

[caitp (gmail), 2016-02-16 22:25:34]

Patchset #4 (id:60001) has been deleted

[caitp (gmail), 2016-02-16 22:47:35]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-02-16 22:47:36]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1700123003/#ps80001
(title: "more oops")

[commit-bot: I haz the power, 2016-02-16 22:47:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1700123003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1700123003/80001

[commit-bot: I haz the power, 2016-02-16 23:38:06]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-02-16 23:38:14]

Description was changed from

==========
[parser] unify metaproperty parsing and require unescaped property name

BUG=v8:4756
LOG=N
R=adamk@chromium.org, littledan@chromium.org, wingo@igalia.com
==========

to

==========
[parser] unify metaproperty parsing and require unescaped property name

BUG=v8:4756
LOG=N
R=adamk@chromium.org, littledan@chromium.org, wingo@igalia.com

Committed: https://crrev.com/fd2edb0ea2b5317c5b27ac3fa40dea9ecf6159f0
Cr-Commit-Position: refs/heads/master@{#34050}
==========

[commit-bot: I haz the power, 2016-02-16 23:38:15]

Patchset 4 (id:??) landed as
https://crrev.com/fd2edb0ea2b5317c5b27ac3fa40dea9ecf6159f0
Cr-Commit-Position: refs/heads/master@{#34050}