Reviewers: wingo,
https://codereview.chromium.org/883823002/diff/20001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/883823002/diff/20001/src/ast.h#newcode1633
src/ast.h:1633: return !is_this() && (!is_resolved() ||
var()->IsValidReference());
On 2015/02/04 10:02:13, wingo wrote:
So it used to be that we would eagerly resolve "this" references, and
we would
fall through to the second case where var()->IsValidReference()
returns false.
But now that we create unresolved "this" references we have to eagerly
prevent
unresolved "this" references from being valid left-hand-sides.
Given that, it seems to me that we need to update Variable to not have
an
is_valid_ref() field, as it's not needed any more -- the only cases I
could see
where it mattered were for "this". (SuperExpression is not a
ThisExpression so
the comment in variables.h is outdated.) Can you give that a try?
Then we
remove the RHS of the && here.
Acknowledged.
Also, I am going to write it as:
return !(is_this() || is_unresolved());
Somehow my mind can parse it more easily that way.
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1178
src/scopes.cc:1178:
On 2015/02/04 10:02:13, wingo wrote:
extra whitespace
Acknowledged.
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1206
src/scopes.cc:1206: // which is not preallocated.
On 2015/02/04 10:02:13, wingo wrote:
Confusing comment. Also I think the conditional below should be
reassociated:
if var->is_this(), then certainly var->is_used() (per the if block
above).
i.e. return var->is_this() || (var->is_used() &&
!var->IsGlobalObjectProperty());
Acknowledged.
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1299
src/scopes.cc:1299: DCHECK(receiver()->scope() == this);
On 2015/02/04 10:02:13, wingo wrote:
the second DCHECK is sufficient, I think, if we add the non-null
DCHECK to
receiver() itself
Acknowledged.
https://codereview.chromium.org/883823002/diff/20001/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode149
src/scopes.h:149: bool is_this = false) {
On 2015/02/04 10:02:13, wingo wrote:
Boolean parameters are an antipattern, especially optional boolean
parameters,
because they read so poorly. Please create a small enum somewhere. I
know that
is_this is used in other parts of the API but it's bad there too.
Acknowledged. Even better: the actual name of the variable is always
passed in, so why not checking that “name ==
ast_value_factory->this_string()”? I am gonna go with that, and it will
save one argument being passed around.
https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode343
src/scopes.h:343: DCHECK(has_this_declaration());
On 2015/02/04 10:02:13, wingo wrote:
DCHECK(receiver_) also perhaps?
Acknowledged.
Description:
Implement proper scoping for "this" in arrow functions
BUG=v8:2700
LOG=Y
Please review this at https://codereview.chromium.org/883823002/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+173, -95 lines):
M src/arm/full-codegen-arm.cc
M src/arm/lithium-codegen-arm.cc
M src/arm64/full-codegen-arm64.cc
M src/arm64/lithium-codegen-arm64.cc
M src/ast.h
M src/compiler/ast-graph-builder.cc
M src/ia32/full-codegen-ia32.cc
M src/ia32/lithium-codegen-ia32.cc
M src/mips/full-codegen-mips.cc
M src/mips/lithium-codegen-mips.cc
M src/mips64/full-codegen-mips64.cc
M src/mips64/lithium-codegen-mips64.cc
M src/parser.cc
M src/ppc/full-codegen-ppc.cc
M src/ppc/lithium-codegen-ppc.cc
M src/preparser.h
M src/scopes.h
M src/scopes.cc
M src/x64/full-codegen-x64.cc
M src/x64/lithium-codegen-x64.cc
M src/x87/full-codegen-x87.cc
M src/x87/lithium-codegen-x87.cc
M test/cctest/test-parsing.cc
A test/mjsunit/harmony/arrow-functions-scoping.js
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
