[interpreter] Support for ES6 class literals.

[interpreter] Support for ES6 class literals.

Port of class literal support from the
ast-graph-builder implementation.

R=rmcilroy@chromium.org,mstarzinger@chromium.org
BUG=v8:4280, v8:4682
LOG=N

Committed: https://crrev.com/1b436ae168ea6e6b164352d85e6b4f8492808e4c
Cr-Commit-Position: refs/heads/master@{#33746}

[oth, 2016-02-04 12:02:43]

Ross, Michi, PTAL, thanks.

[oth, 2016-02-04 12:02:52]

The CQ bit was checked by oth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 12:03:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1666943003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1666943003/1

[oth, 2016-02-04 12:03:51]

Description was changed from

==========
[interpreter] Support for ES6 class literals.

Port of class literal support from ast-graph-builder implementation.

R=rmcilroy@chromium.org,mstarzinger@chromium.org
BUG=v8:4280,v8:4682
LOG=N
==========

to

==========
[interpreter] Support for ES6 class literals.

Port of class literal support from the
ast-graph-builder implementation.

R=rmcilroy@chromium.org,mstarzinger@chromium.org
BUG=v8:4280,v8:4682
LOG=N
==========

[commit-bot: I haz the power, 2016-02-04 12:22:17]

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

[commit-bot: I haz the power, 2016-02-04 12:22:18]

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

[Michael Starzinger, 2016-02-04 13:40:20]

LGTM with nits to address.

https://codereview.chromium.org/1666943003/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1666943003/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:982: FrameStateBeforeAndAfter
states(this);
nit: Shouldn't need a frame state. Can we drop this line?

https://codereview.chromium.org/1666943003/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:984:
environment()->BindAccumulator(BuildLoadObjectField(
nit: This is a dangerous pattern, because C++ does not specify whether
"environment()" or "BuildLoadObjectField()" is evaluated first. So if the latter
modifies the current environment, then we are relying on implementation-specific
behavior here. Can we instead pull out the result into a local variable?

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (left):

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:308: DCHECK(index_ != kInvalidIndex);
nit: Please run "git cl format".

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:847: // Looks up the prototype or initial map of
the JSFunction referenced
nit: s/Looks up/Loads/

https://codereview.chromium.org/1666943003/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1666943003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:18: static const InstanceType
kInstanceTypeDontCare = (InstanceType)-1;
nit: Can we use static_cast instead of C-style case here?

[oth, 2016-02-04 14:00:59]

Thanks, all incorporated.

https://codereview.chromium.org/1666943003/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1666943003/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:982: FrameStateBeforeAndAfter
states(this);
On 2016/02/04 13:40:20, Michael Starzinger wrote:
> nit: Shouldn't need a frame state. Can we drop this line?

Done.

https://codereview.chromium.org/1666943003/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:984:
environment()->BindAccumulator(BuildLoadObjectField(
On 2016/02/04 13:40:20, Michael Starzinger wrote:
> nit: This is a dangerous pattern, because C++ does not specify whether
> "environment()" or "BuildLoadObjectField()" is evaluated first. So if the
latter
> modifies the current environment, then we are relying on
implementation-specific
> behavior here. Can we instead pull out the result into a local variable?

Thanks, aware of the issue. Hadn't thought this instance through.

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (left):

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:308: DCHECK(index_ != kInvalidIndex);
On 2016/02/04 13:40:20, Michael Starzinger wrote:
> nit: Please run "git cl format".

Manually fixed. Current git cl format is not updating this.

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1666943003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:847: // Looks up the prototype or initial map of
the JSFunction referenced
On 2016/02/04 13:40:20, Michael Starzinger wrote:
> nit: s/Looks up/Loads/

Done.

https://codereview.chromium.org/1666943003/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1666943003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:18: static const InstanceType
kInstanceTypeDontCare = (InstanceType)-1;
On 2016/02/04 13:40:20, Michael Starzinger wrote:
> nit: Can we use static_cast instead of C-style case here?

Done.

[rmcilroy, 2016-02-04 14:56:41]

LGTM with a couple of nits

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1249:
builder()->LoadPrototypeOrInitialMap();
nit - chain builders (in functions below too) ?

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1254: // freeze them in strong mode.
Remove the comment about strong mode (we don't support it and AFAIK it is being
ripped out soon)

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1275:
VisitForAccumulatorValueOrTheHole(expr->extends());
nit - newline above (bit of a wall of text otherwise :)).

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1290: EffectResultScope
result_scope(this);
Remove please (or replace with a RegisterAllocationScope)

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1318: // not need to do this for every
property.
I think this comment might make more sense in
VisitClassLiteralStaticPrototypeWithComputedName, WDYT?

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1340: UNREACHABLE();
nit - comment on why this should be unreachable and also add break;

https://codereview.chromium.org/1666943003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1666943003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:8356: 
nit - add // clang-format off / on around the snippet

[oth, 2016-02-04 16:59:48]

Thanks, all incorporated but haven't moved the comment covered with WDYT.

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1249:
builder()->LoadPrototypeOrInitialMap();
On 2016/02/04 14:56:41, rmcilroy wrote:
> nit - chain builders (in functions below too) ?

Done.

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1254: // freeze them in strong mode.
On 2016/02/04 14:56:41, rmcilroy wrote:
> Remove the comment about strong mode (we don't support it and AFAIK it is
being
> ripped out soon)

Done.

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1275:
VisitForAccumulatorValueOrTheHole(expr->extends());
On 2016/02/04 14:56:41, rmcilroy wrote:
> nit - newline above (bit of a wall of text otherwise :)).

Done.

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1290: EffectResultScope
result_scope(this);
On 2016/02/04 14:56:41, rmcilroy wrote:
> Remove please (or replace with a RegisterAllocationScope)

Done.

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1318: // not need to do this for every
property.
On 2016/02/04 14:56:40, rmcilroy wrote:
> I think this comment might make more sense in
> VisitClassLiteralStaticPrototypeWithComputedName, WDYT?

It seems like the right place as it explains the purpose of the path that the if
statement exposes. If you'd still like it moved, I don't feel too strongly about
it.

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1340: UNREACHABLE();
On 2016/02/04 14:56:41, rmcilroy wrote:
> nit - comment on why this should be unreachable and also add break;

Done.

https://codereview.chromium.org/1666943003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1666943003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:8356: 
On 2016/02/04 14:56:41, rmcilroy wrote:
> nit - add // clang-format off / on around the snippet

Done.

[rmcilroy, 2016-02-04 17:37:06]

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1666943003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1318: // not need to do this for every
property.
On 2016/02/04 16:59:48, oth wrote:
> On 2016/02/04 14:56:40, rmcilroy wrote:
> > I think this comment might make more sense in
> > VisitClassLiteralStaticPrototypeWithComputedName, WDYT?
> 
> It seems like the right place as it explains the purpose of the path that the
if
> statement exposes. If you'd still like it moved, I don't feel too strongly
about
> it.

Yeah sorry you are right, it belongs here.

[oth, 2016-02-04 17:45:27]

The CQ bit was checked by oth@chromium.org

[oth, 2016-02-04 17:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1666943003/#ps120001
(title: "Another tweak to cctest.status.")

[commit-bot: I haz the power, 2016-02-04 17:45:35]

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

[commit-bot: I haz the power, 2016-02-04 17:47:15]

Description was changed from

==========
[interpreter] Support for ES6 class literals.

Port of class literal support from the
ast-graph-builder implementation.

R=rmcilroy@chromium.org,mstarzinger@chromium.org
BUG=v8:4280,v8:4682
LOG=N
==========

to

==========
[interpreter] Support for ES6 class literals.

Port of class literal support from the
ast-graph-builder implementation.

R=rmcilroy@chromium.org,mstarzinger@chromium.org
BUG=v8:4280,v8:4682
LOG=N
==========

[commit-bot: I haz the power, 2016-02-04 17:47:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-04 17:47:45]

Description was changed from

==========
[interpreter] Support for ES6 class literals.

Port of class literal support from the
ast-graph-builder implementation.

R=rmcilroy@chromium.org,mstarzinger@chromium.org
BUG=v8:4280,v8:4682
LOG=N
==========

to

==========
[interpreter] Support for ES6 class literals.

Port of class literal support from the
ast-graph-builder implementation.

R=rmcilroy@chromium.org,mstarzinger@chromium.org
BUG=v8:4280,v8:4682
LOG=N

Committed: https://crrev.com/1b436ae168ea6e6b164352d85e6b4f8492808e4c
Cr-Commit-Position: refs/heads/master@{#33746}
==========

[commit-bot: I haz the power, 2016-02-04 17:47:46]

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