Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(201)

Issue 1149133005: [es6] Add TF support for super.property (Closed)

Created:
5 years, 6 months ago by arv (Not doing code reviews)
Modified:
5 years, 6 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[es6] Add TF support for super.property Currently does super.prop (load) and super.method() (call). Like full codegen it uses runtime calls to load the property value. BUG=v8:3330 LOG=N R=mstarzinger@chromium.org, dslomov@chromium.org Committed: https://crrev.com/06c706d84c04fc2e31e993518df9dcefb7543a2d Cr-Commit-Position: refs/heads/master@{#28815}

Patch Set 1 : #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+310 lines, -57 lines) Patch
M src/compiler/ast-graph-builder.h View 2 chunks +13 lines, -0 lines 0 comments Download
M src/compiler/ast-graph-builder.cc View 15 chunks +297 lines, -57 lines 8 comments Download

Messages

Total messages: 24 (10 generated)
arv (Not doing code reviews)
Hi Michi, I started hacking on this. My plan is to do: 1. super.prop using ...
5 years, 6 months ago (2015-05-28 21:56:02 UTC) #3
arv (Not doing code reviews)
Ready for review
5 years, 6 months ago (2015-05-29 20:27:23 UTC) #5
arv (Not doing code reviews)
self review
5 years, 6 months ago (2015-05-29 20:36:59 UTC) #6
arv (Not doing code reviews)
PTAL This is now complete and ready for review.
5 years, 6 months ago (2015-05-29 20:47:32 UTC) #7
arv (Not doing code reviews)
Update to use this_funtion_var instead of home_object_var
5 years, 6 months ago (2015-06-04 17:12:40 UTC) #8
arv (Not doing code reviews)
PTAL This is blocking my work on eval super
5 years, 6 months ago (2015-06-04 17:15:06 UTC) #10
arv (Not doing code reviews)
On 2015/06/04 17:15:06, arv wrote: > PTAL > > This is blocking my work on ...
5 years, 6 months ago (2015-06-04 19:08:43 UTC) #11
Michael Starzinger
LGTM. https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph-builder.cc File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph-builder.cc#newcode2313 src/compiler/ast-graph-builder.cc:2313: if (!property->IsSuperAccess()) { Could this be modeled as ...
5 years, 6 months ago (2015-06-05 13:28:19 UTC) #16
arv (Not doing code reviews)
https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph-builder.cc File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph-builder.cc#newcode2313 src/compiler/ast-graph-builder.cc:2313: if (!property->IsSuperAccess()) { On 2015/06/05 13:28:19, Michael Starzinger wrote: ...
5 years, 6 months ago (2015-06-05 13:50:26 UTC) #17
Michael Starzinger
https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph-builder.cc File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph-builder.cc#newcode2313 src/compiler/ast-graph-builder.cc:2313: if (!property->IsSuperAccess()) { On 2015/06/05 13:50:26, arv wrote: > ...
5 years, 6 months ago (2015-06-05 13:53:24 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149133005/100001
5 years, 6 months ago (2015-06-05 13:59:25 UTC) #20
commit-bot: I haz the power
Committed patchset #1 (id:100001)
5 years, 6 months ago (2015-06-05 14:32:47 UTC) #21
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/06c706d84c04fc2e31e993518df9dcefb7543a2d Cr-Commit-Position: refs/heads/master@{#28815}
5 years, 6 months ago (2015-06-05 14:32:58 UTC) #22
brucedawson
5 years, 6 months ago (2015-06-08 17:24:50 UTC) #24
Message was sent while issue was closed.
https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:2219: Node* value;
It is not clear that 'value' will always be initialized before it is used by
this function. This will be true as long as GetAssignType() always returns one
of the five values listed in the switch statement and this may not be a
long-term stable assumption.

Consider initializing value to nullptr when it is declared, thus making it
trivially obvious that it is always initialized. This is virtually free.

https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:2225: value = nullptr;
i.e.; move this assignment to the variable declaration.

Powered by Google App Engine
This is Rietveld 408576698