Split the AST representation of class properties from object properties

Split the AST representation of class properties from object properties.

This introduces ClassLiteralProperty and a supertype LiteralProperty of
it and ObjectLiteralProperty. It also splits the parsing of the two.
This substiantially clarifies some logic, especially as classes
continue to evolve, and is also about a 2% performance improvement to
parsing either kind of property (since no work is wasted on logic
only necessary for the other kind). Also, it saves a word on
ObjectLiteralProperties.

Committed: https://crrev.com/7bc200c767470b02af5bf3cc12e23ec4bc104e80
Cr-Commit-Position: refs/heads/master@{#39219}

[bakkot, 2016-08-31 23:13:13]

Description was changed from

==========
Split the AST representation of class properties from object properties.

This introduces ClassLiteralProperty and a supertype LiteralProperty of
it and ObjectLiteralProperty. It also splits the parsing of the two.
This is a substantial simplification in logic, especially as classes
continue to evolve, and also about a 2% performance improvement to
parsing either kind of property (since no work is wasted on logic
only necessary for the other kind).
==========

to

==========
Split the AST representation of class properties from object properties.

This introduces ClassLiteralProperty and a supertype LiteralProperty of
it and ObjectLiteralProperty. It also splits the parsing of the two.
This is a substantial simplification in logic, especially as classes
continue to evolve, and also about a 2% performance improvement to
parsing either kind of property (since no work is wasted on logic
only necessary for the other kind). Also, it saves a word on
ObjectLiteralProperties.
==========

[bakkot, 2016-08-31 23:32:28]

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

[commit-bot: I haz the power, 2016-08-31 23:32:31]

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 23:37:22]

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

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

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

[bakkot, 2016-08-31 23:48:42]

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

[commit-bot: I haz the power, 2016-08-31 23:48:52]

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 23:53:28]

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

[commit-bot: I haz the power, 2016-08-31 23:53:29]

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

[bakkot, 2016-09-01 19:08:43]

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

[commit-bot: I haz the power, 2016-09-01 19:08:52]

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-09-01 19:13:22]

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

[commit-bot: I haz the power, 2016-09-01 19:13:23]

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

[bakkot, 2016-09-01 19:13:40]

The CQ bit was unchecked by bakkot@google.com

[bakkot, 2016-09-01 19:15:27]

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

[commit-bot: I haz the power, 2016-09-01 19:15:40]

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-09-01 19:42:10]

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

[commit-bot: I haz the power, 2016-09-01 19:42:11]

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-09-01 20:17:31]

Description was changed from

==========
Split the AST representation of class properties from object properties.

This introduces ClassLiteralProperty and a supertype LiteralProperty of
it and ObjectLiteralProperty. It also splits the parsing of the two.
This is a substantial simplification in logic, especially as classes
continue to evolve, and also about a 2% performance improvement to
parsing either kind of property (since no work is wasted on logic
only necessary for the other kind). Also, it saves a word on
ObjectLiteralProperties.
==========

to

==========
Split the AST representation of class properties from object properties.

This introduces ClassLiteralProperty and a supertype LiteralProperty of
it and ObjectLiteralProperty. It also splits the parsing of the two.
This substiantially clarifies some logic, especially as classes
continue to evolve, and is also about a 2% performance improvement to
parsing either kind of property (since no work is wasted on logic
only necessary for the other kind). Also, it saves a word on
ObjectLiteralProperties.
==========

[bakkot, 2016-09-01 20:18:35]

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

[bakkot, 2016-09-01 20:18:36]

PTAL. This touches a lot of files, but mostly in trivial ways. The interesting
changes are in the AST and parser-base.h/{ParseObjectPropertyDefinition,
ParseClassPropertyDefinition}, and to a lesser extent
parser-base.h/ParsePropertyName.

Not sure if other reviewers are appropriate for the AST changes.

[adamk, 2016-09-01 21:18:23]

Interesting stuff. Initially it looks like a lot of duplication, but it is
interesting to see how much of the post-processing of the return value of
ParsePropertyName is actually disjoint, so it really does seem like this is a
step in the right direction.

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1298
src/ast/ast.h:1298: Expression* value() { return value_; }
Might as well make these const methods while you're here?

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1319
src/ast/ast.h:1319: Expression* key_;
Please leave a blank line above to separate the constructor from the data
properties.

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1339
src/ast/ast.h:1339: Kind kind() { return kind_; }
Consider adding const modifiers to the getters in this class, too.

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1361
src/ast/ast.h:1361: bool emit_store_;
Make this a single-bit bitfield to pack with kind_ and other the bitfield in
LiteralProperty?

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode2692
src/ast/ast.h:2692: Kind kind() { return kind_; }
Make this a const method too.

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

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2046: kind = PropertyKind::kMethodProperty;
If you move this into the LPAREN branch, then you don't need the weird || or
comment in the kNotSet DCHECK in ParsePropertyName.

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2065: Token::Value next = Next();
If you inline the Next() call into the call to ReportUnexpectedToken, you don't
need the curlies around this case block (also saves a line). Similar comment for
other calls to ReportUnexpectedToken().

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2076: method_kind |= MethodKind::kGenerator;
I'm not sure MethodKind is buying us a whole lot of readability. The only
callers of CheckClassMethodName already have bools available called
is_generator, is_async, and is_static, and already have to pass a bunch of bools
in the correct order to ParsePropertyName. And then we turn them back into bools
inside!

What would you think of just having CheckClassMethodName take bools, and getting
rid of all these bitwise operations?

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2319: case PropertyKind::kNotSet: {
Can you explain why it's better to have duplication of this error-reporting
rather than handling it in ParsePropertyName? I suspect I'm missing something.

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5541: void
Parser::SetFunctionNameFromPropertyName(ClassLiteralProperty* property,
Duplicating this is kind of sad...I wonder why we don't just create the right
function name for getter properties to begin with. We don't need to change that
logic in this CL, I guess. But it might help make this duplication less bad.

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

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser.h#ne...
src/parsing/parser.h:711: V8_INLINE static Expression* GetPropertyValue(
Can't you just take a LiteralProperty* instead?

[bakkot, 2016-09-01 23:24:31]

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1298
src/ast/ast.h:1298: Expression* value() { return value_; }
On 2016/09/01 at 21:18:22, adamk wrote:
> Might as well make these const methods while you're here?

Done.

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1319
src/ast/ast.h:1319: Expression* key_;
On 2016/09/01 at 21:18:23, adamk wrote:
> Please leave a blank line above to separate the constructor from the data
properties.

Done.

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1339
src/ast/ast.h:1339: Kind kind() { return kind_; }
On 2016/09/01 at 21:18:22, adamk wrote:
> Consider adding const modifiers to the getters in this class, too.

Done.

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode1361
src/ast/ast.h:1361: bool emit_store_;
On 2016/09/01 at 21:18:23, adamk wrote:
> Make this a single-bit bitfield to pack with kind_ and other the bitfield in
LiteralProperty?

Sounded like this wasn't worth doing, so I'm leaving as-is for the moment.

https://codereview.chromium.org/2302643002/diff/80001/src/ast/ast.h#newcode2692
src/ast/ast.h:2692: Kind kind() { return kind_; }
On 2016/09/01 at 21:18:22, adamk wrote:
> Make this a const method too.

Done.

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

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2046: kind = PropertyKind::kMethodProperty;
On 2016/09/01 at 21:18:23, adamk wrote:
> If you move this into the LPAREN branch, then you don't need the weird || or
comment in the kNotSet DCHECK in ParsePropertyName.

Done. This was historical, not really intentional. It had the effect that
ParsePropertyName would necessarily set kind to either Method or Accessor, so
that if the name was followed by a colon it would fail in the Method branch
below. With this change, 'static x :' will be interpreted as a ValueProperty by
ParsePropertyName (since it will only see the 'x :' part), and then the error
will be thrown by hitting the ValueProperty branch.

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2065: Token::Value next = Next();
On 2016/09/01 at 21:18:23, adamk wrote:
> If you inline the Next() call into the call to ReportUnexpectedToken, you
don't need the curlies around this case block (also saves a line). Similar
comment for other calls to ReportUnexpectedToken().

Done.

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2076: method_kind |= MethodKind::kGenerator;
On 2016/09/01 at 21:18:23, adamk wrote:
> I'm not sure MethodKind is buying us a whole lot of readability. The only
callers of CheckClassMethodName already have bools available called
is_generator, is_async, and is_static, and already have to pass a bunch of bools
in the correct order to ParsePropertyName. And then we turn them back into bools
inside!
> 
> What would you think of just having CheckClassMethodName take bools, and
getting rid of all these bitwise operations?

Done.

Yeah, that's much better. MethodKind requires much more code than it's worth.
It's been getting progressively less useful with these patches.

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2319: case PropertyKind::kNotSet: {
On 2016/09/01 at 21:18:23, adamk wrote:
> Can you explain why it's better to have duplication of this error-reporting
rather than handling it in ParsePropertyName? I suspect I'm missing something.

Discussed offline. The real reason is that it makes adding class fields later
easier. The ideological reason is that ParsePropertyName shouldn't really be
throwing errors for unexpected tokens past the end of the property name; that's
the responsibility of whatever wants to consume that token.

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5541: void
Parser::SetFunctionNameFromPropertyName(ClassLiteralProperty* property,
On 2016/09/01 at 21:18:23, adamk wrote:
> Duplicating this is kind of sad...I wonder why we don't just create the right
function name for getter properties to begin with. We don't need to change that
logic in this CL, I guess. But it might help make this duplication less bad.

I'm going to leave this as-is for the moment. I'm going to put together another
CL cleaning up the logic for inferring function names from property names
everywhere, but it doesn't make sense as part of this patch. I misread this code
initially; this also sets names for things like ({x: function(){}}), although
I'm pretty sure it doesn't do anything for shorthand methods.

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

https://codereview.chromium.org/2302643002/diff/80001/src/parsing/parser.h#ne...
src/parsing/parser.h:711: V8_INLINE static Expression* GetPropertyValue(
On 2016/09/01 at 21:18:23, adamk wrote:
> Can't you just take a LiteralProperty* instead?

Whoops. Done.

[adamk, 2016-09-01 23:30:30]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org

[adamk, 2016-09-01 23:30:31]

This lgtm, but I'd like to give marja@ a look at this too since she reviewed the
previous CL in this series.

Also +bmeurer for OWNERS on the compiler backends.

[Benedikt Meurer, 2016-09-02 05:12:13]

bmeurer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2016-09-02 05:12:13]

bmeurer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2016-09-02 05:12:14]

Michi should have a look at the compiler changes.

[marja, 2016-09-02 08:02:10]

lgtm

[marja, 2016-09-02 08:02:52]

bmeurer@: mstarzinger@ is ooo, can you suggest somebody who can review this
before he's back?

[adamk, 2016-09-02 17:08:03]

On 2016/09/02 08:02:52, marja wrote:
> bmeurer@: mstarzinger@ is ooo, can you suggest somebody who can review this
> before he's back?

Also note that the compiler changes themselves are (from my point of view at
least) pure goodness. They tighten up the invariants about what can be inside a
ClassLiteral, letting the C++ type system do the job of enforcing those
invariants instead of a relying on a runtime UNREACHABLE() check.

[Michael Starzinger, 2016-09-05 16:13:50]

LGTM. I like it. Note that I didn't look at the changes in the "parsing"
directory, I assume Adam covered that already. Also sorry for late review, I was
on vacation.

[bakkot, 2016-09-06 17:03:31]

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

[commit-bot: I haz the power, 2016-09-06 17:03:41]

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-09-06 17:33:47]

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

[commit-bot: I haz the power, 2016-09-06 17:33:48]

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-09-06 17:41:15]

The CQ bit was checked by bakkot@google.com

[bakkot, 2016-09-06 17:41:15]

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

[commit-bot: I haz the power, 2016-09-06 17:41:18]

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-09-06 17:43:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-06 17:43:56]

Description was changed from

==========
Split the AST representation of class properties from object properties.

This introduces ClassLiteralProperty and a supertype LiteralProperty of
it and ObjectLiteralProperty. It also splits the parsing of the two.
This substiantially clarifies some logic, especially as classes
continue to evolve, and is also about a 2% performance improvement to
parsing either kind of property (since no work is wasted on logic
only necessary for the other kind). Also, it saves a word on
ObjectLiteralProperties.
==========

to

==========
Split the AST representation of class properties from object properties.

This introduces ClassLiteralProperty and a supertype LiteralProperty of
it and ObjectLiteralProperty. It also splits the parsing of the two.
This substiantially clarifies some logic, especially as classes
continue to evolve, and is also about a 2% performance improvement to
parsing either kind of property (since no work is wasted on logic
only necessary for the other kind). Also, it saves a word on
ObjectLiteralProperties.

Committed: https://crrev.com/7bc200c767470b02af5bf3cc12e23ec4bc104e80
Cr-Commit-Position: refs/heads/master@{#39219}
==========

[commit-bot: I haz the power, 2016-09-06 17:43:58]

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