Cleanup scope resolution

Cleanup scope resolution

BUG=v8:5209
Committed: https://crrev.com/e77a78cd2d17d4b3b0bf2db79c5d83bf4b2150c2
Cr-Commit-Position: refs/heads/master@{#38580}

[Toon Verwaest, 2016-08-11 09:00:50]

verwaest@chromium.org changed reviewers:
+ marja@chromium.org

[Toon Verwaest, 2016-08-11 09:00:50]

ptal

[marja, 2016-08-11 09:37:35]

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

https://codereview.chromium.org/2230323004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1301: return var;
As discussed offline, this changes the behavior wrt shadowing the function var.

https://codereview.chromium.org/2230323004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1321: // scope,
pls re-wrap this

https://codereview.chromium.org/2230323004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1466: // Anything that was bound
What is this comment? (Not introduced by this CL but since you're touching this,
can you either remove the comment or modify it so that it makes sense.)

[Toon Verwaest, 2016-08-11 10:50:46]

Addressed comments, added test.

[Toon Verwaest, 2016-08-11 11:33:01]

Addressed comment. I'm not 100% sure it's better, since we can't reresolve that
VariableProxy to the matching Var potentially. Now it doesn't fail anything
anymore since FetchFreeVariables now skips already-resolved variables. When it
wasn't doing that, it did fail...
I'll land it like this for now, we can always change in a follow-up CL if you
change your mind :)

[Toon Verwaest, 2016-08-11 11:33:23]

Oops, totally the wrong CL for that comment, ignore me!

[marja, 2016-08-11 11:44:39]

lgtm

[Toon Verwaest, 2016-08-11 12:55:12]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2016-08-11 12:55:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 12:56:28]

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

[commit-bot: I haz the power, 2016-08-11 12:56:29]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10812)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/21631)

[Toon Verwaest, 2016-08-11 12:58:42]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-11 12:58:43]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2230323004/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-11 12:58:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 13:25:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-11 13:26:12]

Description was changed from

==========
Cleanup scope resolution

BUG=v8:5209
==========

to

==========
Cleanup scope resolution

BUG=v8:5209

Committed: https://crrev.com/e77a78cd2d17d4b3b0bf2db79c5d83bf4b2150c2
Cr-Commit-Position: refs/heads/master@{#38580}
==========

[commit-bot: I haz the power, 2016-08-11 13:26:13]

Patchset 6 (id:??) landed as
https://crrev.com/e77a78cd2d17d4b3b0bf2db79c5d83bf4b2150c2
Cr-Commit-Position: refs/heads/master@{#38580}

[adamk, 2016-08-11 18:19:18]

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

[adamk, 2016-08-11 18:19:19]

I'm not clear on what this CL cleans up (I actually find it harder to read, with
the "with" and "eval" bits separated. Can you say more about the motivation?

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1325: DCHECK(!is_with_scope());
If you want this DCHECK to be useful it's gotta go the one above :)

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1328: if (calls_sloppy_eval() && is_declaration_scope() &&
!is_script_scope()) {
This is no longer guarded by "name_can_be_shadowed"...was that intentional?

[Toon Verwaest, 2016-08-12 05:16:58]

I find it cleaner at least. It moves branches to where they are relevant, and
removes conditions where irrelevant. It reduces control flow to what's necessary
so it's also more efficient.

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1325: DCHECK(!is_with_scope());
On 2016/08/11 18:19:19, adamk wrote:
> If you want this DCHECK to be useful it's gotta go the one above :)

I don't see the issue with checking both? I can remove either if you insist.

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1328: if (calls_sloppy_eval() && is_declaration_scope() &&
!is_script_scope()) {
On 2016/08/11 18:19:19, adamk wrote:
> This is no longer guarded by "name_can_be_shadowed"...was that intentional?

It is. Either we have a var null since outer scope, in which case name can be
shadowed, or we returned early above.

See, the old code wasn't easy to understand ;-)

[adamk, 2016-08-15 17:45:00]

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1325: DCHECK(!is_with_scope());
On 2016/08/12 05:16:58, Toon Verwaest wrote:
> On 2016/08/11 18:19:19, adamk wrote:
> > If you want this DCHECK to be useful it's gotta go the one above :)
> 
> I don't see the issue with checking both? I can remove either if you insist.

What I mean is that a with scope will fail on the previous line without
revealing its with-scope-ness. I think there's value in both DCHECKs, but only
in reverse order.

https://codereview.chromium.org/2230323004/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1328: if (calls_sloppy_eval() && is_declaration_scope() &&
!is_script_scope()) {
On 2016/08/12 05:16:58, Toon Verwaest wrote:
> On 2016/08/11 18:19:19, adamk wrote:
> > This is no longer guarded by "name_can_be_shadowed"...was that intentional?
> 
> It is. Either we have a var null since outer scope, in which case name can be
> shadowed, or we returned early above.
> 
> See, the old code wasn't easy to understand ;-)

Point taken :)