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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 791603003: WIP context-allocation for “this” in arrow functions

Created:
6 years ago by aperez
Modified:
5 years, 11 months ago
Reviewers:
wingo
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

WIP context-allocation for “this” in arrow functions BUG=

Patch Set 1 #

Patch Set 2 : WIP v2 #

Patch Set 3 : WIP v3 #

Total comments: 18

Patch Set 4 : Working for x64 (codegen missing for other arches) #

Patch Set 5 : Clean up a couple of comments, otherwise same as previous patch set #

Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -17 lines) Patch
M src/ast-numbering.cc View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M src/bailout-reason.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/heap/heap.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M src/scopes.cc View 1 2 3 3 chunks +42 lines, -9 lines 0 comments Download
M src/x64/full-codegen-x64.cc View 1 2 3 4 3 chunks +22 lines, -4 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 1 chunk +7 lines, -3 lines 0 comments Download
A test/mjsunit/harmony/arrow-functions-scoping.js View 1 2 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (1 generated)
aperez
6 years ago (2014-12-09 19:17:45 UTC) #2
wingo
Looking good, some comments before you write the other backends https://codereview.chromium.org/791603003/diff/40001/src/ast-numbering.cc File src/ast-numbering.cc (right): https://codereview.chromium.org/791603003/diff/40001/src/ast-numbering.cc#newcode562 ...
5 years, 11 months ago (2015-01-15 10:09:06 UTC) #3
aperez
5 years, 11 months ago (2015-01-15 16:58:03 UTC) #4 
https://codereview.chromium.org/791603003/diff/40001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/791603003/diff/40001/src/ast-numbering.cc#new...
src/ast-numbering.cc:562: // TODO(aperez): Add support in Crankshaft for arrow
functions.
On 2015/01/15 10:09:05, wingo wrote:
> Let's file a bug for the tasks needed in Crankshaft, and mention the bug here.

Done.

https://codereview.chromium.org/791603003/diff/40001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/791603003/diff/40001/src/bailout-reason.h#new...
src/bailout-reason.h:23: V(kCapturedThis, "Receiver is captured in the context")
                     \
On 2015/01/15 10:09:05, wingo wrote:
> Please place in alphabetical order.  Tx :)

Done.

https://codereview.chromium.org/791603003/diff/40001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/791603003/diff/40001/src/scopes.cc#newcode612
src/scopes.cc:612: // Arrow functions can cause the receiver to be collected in
the context.
On 2015/01/15 10:09:06, wingo wrote:
> "allocated".  And really it's "copied to" the context -- the receiver is still
> in the stack slot from the outer function's perspective.

Done.

https://codereview.chromium.org/791603003/diff/40001/src/scopes.cc#newcode1206
src/scopes.cc:1206: if (inner->scope_uses_this_) {
On 2015/01/15 10:09:06, wingo wrote:
> Why do we need the whole list of these?  At some point this should be
refactored
> to be a bitfield, if this propagation is really needed.

I have been trying to factor out the flag propagations, or move some of them
into Scope::FinalizeBlockScope(), but either the result ended up being far from
obvious (and quite difficult to understand when reading the code), or the chunk
of code being ever bigger than it is). So, I think the best is to leave mostly
as-is, and plan to change those flags to a bitfield later — and that will make
it possible to simplify the code.

https://codereview.chromium.org/791603003/diff/40001/src/scopes.cc#newcode1233
src/scopes.cc:1233: }
On 2015/01/15 10:09:05, wingo wrote:
> These propagations should run for all scopes

Done.

https://codereview.chromium.org/791603003/diff/40001/src/scopes.cc#newcode1361
src/scopes.cc:1361: // "this", it must be copied in the context, from where it
will be looked
On 2015/01/15 10:09:06, wingo wrote:
> s/it must be copied in/the receiver must be copied to/

Done.

https://codereview.chromium.org/791603003/diff/40001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/791603003/diff/40001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:117: // of the parent scope, so this does not need
to be done for those.
On 2015/01/15 10:09:06, wingo wrote:
> It's not necessarily the receiver from the parent scope -- the parent scope
> could be a block scope, or another arrow function; anyway let's be more
precise
> with comments :)

Done.

https://codereview.chromium.org/791603003/diff/40001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:225: if (info->scope()->inner_uses_this()) {
On 2015/01/15 10:09:06, wingo wrote:
> Let's factor this into the loop below

Done.

https://codereview.chromium.org/791603003/diff/40001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:2763: // is a sloppy mode method, or by the function
prologue it is an arrow
On 2015/01/15 10:09:06, wingo wrote:
> "if it is"

Done.

Powered by Google App Engine
This is Rietveld 408576698