Cleanups needed for this-scoping in arrow functions

Cleanups needed for this-scoping in arrow functions

Remove Variable::IsValidReference(), and the Variable::is_valid_ref_
member: This was "false" only for "this", and for internal variables.
For the first, VariableProxy::is_this() can be used for the check
instead; and for internal variables, it is guaranteed they they will
not be written to (because the V8 code does not do it, and they are
not accessible from JavaScript).

The "bool is_this" parameter of VariableProxy() constructor is
changed to use Variable::Kind. This will allow to later on adding
a parameter to create unresolved variables of any kind, which in
turn will be used to make references to "this" initially unresolved,
and use the existing variable resolution mechanics for "this".

BUG=v8:2700
LOG=N

Committed: https://crrev.com/00844d466be3d809b95d439fdd14566601780458
Cr-Commit-Position: refs/heads/master@{#27404}

[aperez, 2015-03-23 14:13:03]

aperez@igalia.com changed reviewers:
+ wingo@igalia.com

[wingo, 2015-03-23 15:11:14]

Some nits.  Regarding the log, LOG=N I think as it's just a refactor, and do
remove the bits about changes that aren't included.  Remove the last para too as
it just restates the google c++ style guide.

https://codereview.chromium.org/1024703004/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1024703004/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:130: info->scope()->has_this_declaration()) {
This will change the behavior of "this" scoping in arrow functions already, no? 
And not yet to the correct behavior.  If this is the case, please remove the
codegen changes from this patch; we'll include them in the next one.

https://codereview.chromium.org/1024703004/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1024703004/diff/1/src/scopes.h#newcode349
src/scopes.h:349: bool has_this_declaration() const {
Given that we won't include the codegen changes in this one, probably should
punt this bit too as it's not used.

[aperez, 2015-03-23 18:49:44]

On 2015/03/23 15:11:14, wingo wrote:
> Some nits.  Regarding the log, LOG=N I think as it's just a refactor, and do
> remove the bits about changes that aren't included.  Remove the last para too
as
> it just restates the google c++ style guide.

Ack.

> https://codereview.chromium.org/1024703004/diff/1/src/arm/full-codegen-arm.cc
> File src/arm/full-codegen-arm.cc (right):
> 
>
https://codereview.chromium.org/1024703004/diff/1/src/arm/full-codegen-arm.cc...
> src/arm/full-codegen-arm.cc:130: info->scope()->has_this_declaration()) {
> This will change the behavior of "this" scoping in arrow functions already,
no? 
> And not yet to the correct behavior.  If this is the case, please remove the
> codegen changes from this patch; we'll include them in the next one.

Ack.

Right, this changes the for arrow functions. The updated version has
Scope::has_this_declaration(), and the codegen changes removed. Now it
is only the cleanups, without any addition.

> https://codereview.chromium.org/1024703004/diff/1/src/scopes.h
> File src/scopes.h (right):
> 
> https://codereview.chromium.org/1024703004/diff/1/src/scopes.h#newcode349
> src/scopes.h:349: bool has_this_declaration() const {
> Given that we won't include the codegen changes in this one, probably should
> punt this bit too as it's not used.

Ack.

[wingo, 2015-03-24 08:50:21]

non-owner LGTM with nit.  The nit is to trim the log further, keeping only the
paragraphs that have the bullet points, and remove the bullet points themselves.

[aperez, 2015-03-24 09:21:46]

aperez@igalia.com changed reviewers:
+ marja@chromium.org, svenpanne@chromium.org

[arv (Not doing code reviews), 2015-03-24 11:03:40]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-03-24 11:03:41]

LGTM

[aperez, 2015-03-24 12:46:37]

The CQ bit was checked by aperez@igalia.com

[commit-bot: I haz the power, 2015-03-24 12:46:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1024703004/20001

[commit-bot: I haz the power, 2015-03-24 13:08:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-24 13:08:44]

Patchset 2 (id:??) landed as
https://crrev.com/00844d466be3d809b95d439fdd14566601780458
Cr-Commit-Position: refs/heads/master@{#27404}