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.

Committed: https://crrev.com/6dd26c729584024e17a05a2a76b319d4aecdc138
Cr-Commit-Position: refs/heads/master@{#39027}

[bakkot, 2016-08-25 18:44:51]

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

[commit-bot: I haz the power, 2016-08-25 18:44:58]

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-25 18:47:28]

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

[commit-bot: I haz the power, 2016-08-25 18:47:29]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11632)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7699)

[bakkot, 2016-08-25 19:21:31]

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

[commit-bot: I haz the power, 2016-08-25 19:21:34]

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-25 19:24:43]

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

[commit-bot: I haz the power, 2016-08-25 19:24:45]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22636)

[bakkot, 2016-08-25 19:31:00]

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

[commit-bot: I haz the power, 2016-08-25 19:31:03]

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-25 19:34:43]

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

[commit-bot: I haz the power, 2016-08-25 19:34:44]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22639)

[bakkot, 2016-08-26 17:46:45]

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 (2%)
faster in some cases, 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.
==========

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 (2%)
faster in some cases, 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.
==========

[bakkot, 2016-08-26 17:46:45]

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

[bakkot, 2016-08-26 17:47:00]

PTAL

[bakkot, 2016-08-26 18:47:37]

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 (2%)
faster in some cases, 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.
==========

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, 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.
==========

[bakkot, 2016-08-26 22:13:35]

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, 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.
==========

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

[Dan Ehrenberg, 2016-08-27 00:09:00]

I like this new logic. A couple minor points.

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

https://codereview.chromium.org/2278153004/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:1095: enum PropertyKind {
Nit: While you're at it, what if you made this a class enum so the namespace
isn't polluted?

https://codereview.chromium.org/2278153004/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:1956: break;  // This must be an accessor.
Any way you can avoid the duplication between this part and lines 1912? Looks
like the difference only comes after this switch statement.

https://codereview.chromium.org/2278153004/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2265: return impl()->EmptyObjectLiteralProperty();
Does the compiler require you to keep this statement in?

[bakkot, 2016-08-27 01:51:54]

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

https://codereview.chromium.org/2278153004/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:1095: enum PropertyKind {
On 2016/08/27 at 00:09:00, Dan Ehrenberg wrote:
> Nit: While you're at it, what if you made this a class enum so the namespace
isn't polluted?

Ah, good thought. Done.

https://codereview.chromium.org/2278153004/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:1956: break;  // This must be an accessor.
On 2016/08/27 at 00:09:00, Dan Ehrenberg wrote:
> Any way you can avoid the duplication between this part and lines 1912? Looks
like the difference only comes after this switch statement.

I've factored it out into a method. It's not a huge win, because the
control-flow part still takes up some space, and can't be factored out except
into a macro.

https://codereview.chromium.org/2278153004/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2265: return impl()->EmptyObjectLiteralProperty();
On 2016/08/27 at 00:09:00, Dan Ehrenberg wrote:
> Does the compiler require you to keep this statement in?

Unfortunately yes, at least on some platforms. I initially left it out and it
built fine on my machine, but failed the Windows trybots.

[marja, 2016-08-29 08:07:46]

lgtm % comment, but pls wait for littledan@'s review before landing.

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

https://codereview.chromium.org/2278153004/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:1994: if (peek() != Token::COLON && *kind ==
PropertyKind::kValueProperty) {
This is still duplicate logic; can this call SetPropertyKindFromToken? At least
that way we'd have the "token -> property kind" correspondence in one place
only.

[bakkot, 2016-08-29 19:54:50]

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

https://codereview.chromium.org/2278153004/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:1994: if (peek() != Token::COLON && *kind ==
PropertyKind::kValueProperty) {
On 2016/08/29 at 08:07:46, marja wrote:
> This is still duplicate logic; can this call SetPropertyKindFromToken? At
least that way we'd have the "token -> property kind" correspondence in one
place only.

Done. It's a little unpleasant, and strictly speaking we still have that
correspondence in the "static (" case, but that case should go away if the next
patch splitting object and class property parsing goes in.

[Dan Ehrenberg, 2016-08-30 22:31:20]

lgtm

[bakkot, 2016-08-30 23:01:01]

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

[commit-bot: I haz the power, 2016-08-30 23:01:07]

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-30 23:27:56]

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

[commit-bot: I haz the power, 2016-08-30 23:27:57]

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-08-30 23:55:09]

The CQ bit was checked by bakkot@google.com

[bakkot, 2016-08-30 23:55:10]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2278153004/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-30 23:55:16]

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-30 23:57:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-30 23:58:02]

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

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.

Committed: https://crrev.com/6dd26c729584024e17a05a2a76b319d4aecdc138
Cr-Commit-Position: refs/heads/master@{#39027}
==========

[commit-bot: I haz the power, 2016-08-30 23:58:04]

Patchset 7 (id:??) landed as
https://crrev.com/6dd26c729584024e17a05a2a76b319d4aecdc138
Cr-Commit-Position: refs/heads/master@{#39027}

[adamk, 2016-08-31 00:38:35]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2295743003/ by adamk@chromium.org.

The reason for reverting is: Fails to reject "{*foo: 1}" as an object literal,
found
by the fuzzer:

https://build.chromium.org/p/client.v8/builders/V8%20Fuzzer/builds/12315/step....

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

On 2016/08/31 at 00:38:35, adamk wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2295743003/ by adamk@chromium.org.
> 
> The reason for reverting is: Fails to reject "{*foo: 1}" as an object literal,
found
> by the fuzzer:
> 
>
https://build.chromium.org/p/client.v8/builders/V8%20Fuzzer/builds/12315/step....

Relanding at https://codereview.chromium.org/2300503002