Refactor object/class literal property name parsing

Refactor object/class literal property name parsing

This patch arranges that property names are parsed in a single pass,
reporting the name as well as the type of the property, instead of
parsing qualifiers like 'static' or 'get' initially as names and then
re-parsing. This change is easier to reason about, very slightly (4%)
faster in some cases (although slower in other, less common ones, though
this slowdown will be fixed in an upcoming patch), and is a prerequisite
for separating the parsing of object and class literal properties, which
will become increasingly important as ECMAScript adds more class features.

This is a reland of https://codereview.chromium.org/2278153004/,
which fixes the issue causing the revert and adds more tests.

Committed: https://crrev.com/8d5a267b19a5239afbeebc450a197f18231aed78
Cr-Commit-Position: refs/heads/master@{#39056}

[bakkot, 2016-08-31 01:20:47]

bakkot@google.com changed reviewers:
+ adamk@chromium.org, littledan@chromium.org

[bakkot, 2016-08-31 01:22:39]

This patch now properly prohibits ({*x:0}); thanks, fuzzer.

I guess I shouldn't assume we have full test coverage, but at least it's a
little more complete now.

[Dan Ehrenberg, 2016-08-31 15:41:48]

On 2016/08/31 01:22:39, bakkot wrote:
> This patch now properly prohibits ({*x:0}); thanks, fuzzer.
> 
> I guess I shouldn't assume we have full test coverage, but at least it's a
> little more complete now.

lgtm

[bakkot, 2016-08-31 16:39:11]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 16:39:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 16:41:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 16:41:05]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22954)

[adamk, 2016-08-31 17:37:08]

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

https://codereview.chromium.org/2300503002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1899: DCHECK(!*is_computed_name);
Add a DCHECK for the initial state of kind?

https://codereview.chromium.org/2300503002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1938: if (token == Token::IDENTIFIER && !*is_generator
&& !*is_async) {
Can you add a comment here explaining why this only handles Token::IDENTIFIER
and not other tokens? I had to read through a couple of times to understand that
this is for parsing get/set before an IdentifierName (at first it looked like
weird duplication).

https://codereview.chromium.org/2300503002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:2004: if (*kind == PropertyKind::kValueProperty) {
As discussed offline, I'd find this clearer if there were some sentinel value
for kind: seeing a check for a particular kind followed by a call to
SetPropetyKindFromToken seems...wrong.

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

https://codereview.chromium.org/2300503002/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:2917: "static x(){}",
Please add some tests with async (you'll have to add the
kAllowHarmonyAsyncWhatever flag to the RunParserSyncTest call).

https://codereview.chromium.org/2300503002/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:2989: "get : 0",
Maybe some positive async tests too?

[bakkot, 2016-08-31 19:46:28]

Addressed comments. Also set mjsunit/big-object-literal to SKIP on MacOS, since
it was failing for reasons which were not the fault of this patch (v8:5333).

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

https://codereview.chromium.org/2300503002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1899: DCHECK(!*is_computed_name);
On 2016/08/31 at 17:37:07, adamk wrote:
> Add a DCHECK for the initial state of kind?

Done.

https://codereview.chromium.org/2300503002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1938: if (token == Token::IDENTIFIER && !*is_generator
&& !*is_async) {
On 2016/08/31 at 17:37:07, adamk wrote:
> Can you add a comment here explaining why this only handles Token::IDENTIFIER
and not other tokens? I had to read through a couple of times to understand that
this is for parsing get/set before an IdentifierName (at first it looked like
weird duplication).

Done.

https://codereview.chromium.org/2300503002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:2004: if (*kind == PropertyKind::kValueProperty) {
On 2016/08/31 at 17:37:07, adamk wrote:
> As discussed offline, I'd find this clearer if there were some sentinel value
for kind: seeing a check for a particular kind followed by a call to
SetPropetyKindFromToken seems...wrong.

Done.

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

https://codereview.chromium.org/2300503002/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:2917: "static x(){}",
On 2016/08/31 at 17:37:07, adamk wrote:
> Please add some tests with async (you'll have to add the
kAllowHarmonyAsyncWhatever flag to the RunParserSyncTest call).

Done.

https://codereview.chromium.org/2300503002/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:2989: "get : 0",
On 2016/08/31 at 17:37:07, adamk wrote:
> Maybe some positive async tests too?

Done.

[bakkot, 2016-08-31 19:47:39]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 19:47:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 20:14:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 20:14:49]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-08-31 20:43:44]

lgtm

[Dan Ehrenberg, 2016-08-31 20:46:37]

lgtm

[bakkot, 2016-08-31 21:10:29]

The CQ bit was checked by bakkot@google.com

[commit-bot: I haz the power, 2016-08-31 21:10:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 21:13:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-31 21:14:21]

Description was changed from

==========
Refactor object/class literal property name parsing

This patch arranges that property names are parsed in a single pass,
reporting the name as well as the type of the property, instead of
parsing qualifiers like 'static' or 'get' initially as names and then
re-parsing. This change is easier to reason about, very slightly (4%)
faster in some cases (although slower in other, less common ones, though
this slowdown will be fixed in an upcoming patch), and is a prerequisite
for separating the parsing of object and class literal properties, which
will become increasingly important as ECMAScript adds more class features.

This is a reland of https://codereview.chromium.org/2278153004/,
which fixes the issue causing the revert and adds more tests.
==========

to

==========
Refactor object/class literal property name parsing

This patch arranges that property names are parsed in a single pass,
reporting the name as well as the type of the property, instead of
parsing qualifiers like 'static' or 'get' initially as names and then
re-parsing. This change is easier to reason about, very slightly (4%)
faster in some cases (although slower in other, less common ones, though
this slowdown will be fixed in an upcoming patch), and is a prerequisite
for separating the parsing of object and class literal properties, which
will become increasingly important as ECMAScript adds more class features.

This is a reland of https://codereview.chromium.org/2278153004/,
which fixes the issue causing the revert and adds more tests.

Committed: https://crrev.com/8d5a267b19a5239afbeebc450a197f18231aed78
Cr-Commit-Position: refs/heads/master@{#39056}
==========

[commit-bot: I haz the power, 2016-08-31 21:14:22]

Patchset 3 (id:??) landed as
https://crrev.com/8d5a267b19a5239afbeebc450a197f18231aed78
Cr-Commit-Position: refs/heads/master@{#39056}