[parser] Remove hoist_scope from DeclarationDescriptor

[parser] Remove hoist_scope from DeclarationDescriptor

The hoist_scope member of DeclarationDescriptor was only used to pass the function
scope for declaration of parameters containing sloppy evals, for example:

  function f(x = eval("var y")) { }

In cases like this, "x" is declared in the function scope but "y" is declared in an inner scope.
Rather than passing the function scope as "hoist_scope", we simply ask for the outer_scope()
of the inner scope as needed in PatternRewriter.

This reduces the cognitive overhead of understanding what a DeclarationDescriptor has; for
example, it removes some dead code from the PreParser which never has to deal
with a situation like the example above.

Review-Url: https://codereview.chromium.org/2662183002
Cr-Commit-Position: refs/heads/master@{#42861}
Committed: https://chromium.googlesource.com/v8/v8/+/59b8496c81eda6da774af2991a8cf641797a754e

[adamk, 2017-01-31 19:59:31]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 19:59:39]

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

[adamk, 2017-01-31 20:20:10]

Description was changed from

==========
[parser] WIP: Remove hoist_scope from DeclarationDescriptor
==========

to

==========
[parser] Remove hoist_scope from DeclarationDescriptor

The hoist_scope member of DeclarationDescriptor was only used to pass the
function
scope for declaration of parameters containing sloppy evals, for example:

  function f(x = eval("var y")) { }

In cases like this, "x" is declared in the function scope but "y" is declared in
an inner scope.
Rather than passing the function scope as "hoist_scope", we simply ask for the
outer_scope()
of the inner scope as needed in PatternRewriter.

This reduces the cognitive overhead of understanding what a
DeclarationDescriptor has; for
example, it removes some dead code from the PreParser which never has to deal
with a situation like the example above.
==========

[adamk, 2017-01-31 20:20:18]

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

[commit-bot: I haz the power, 2017-01-31 20:25:58]

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

[commit-bot: I haz the power, 2017-01-31 20:25:59]

Dry run: This issue passed the CQ dry run.

[neis, 2017-02-01 08:55:24]

lgtm, very nice

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:147: outer_function_scope =
descriptor_->scope->outer_scope();
Can you add a DCHECK here saying that it's a function scope?

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:343:
DCHECK(scope()->outer_scope()->is_function_scope());
Hmm I guess it's already here because scope() is always descriptor_->scope?

[adamk, 2017-02-01 16:22:15]

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:147: outer_function_scope =
descriptor_->scope->outer_scope();
On 2017/02/01 08:55:24, neis wrote:
> Can you add a DCHECK here saying that it's a function scope?

I could add another one, but it does feel kind of redundant. Let me know what
you think of my suggestion below.

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:343:
DCHECK(scope()->outer_scope()->is_function_scope());
On 2017/02/01 08:55:24, neis wrote:
> Hmm I guess it's already here because scope() is always descriptor_->scope?

Another option would be to call this function something like
GetScopeForParameterContainingEval() and have it return the scope.

[neis, 2017-02-01 16:26:42]

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2662183002/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:343:
DCHECK(scope()->outer_scope()->is_function_scope());
On 2017/02/01 16:22:15, adamk wrote:
> On 2017/02/01 08:55:24, neis wrote:
> > Hmm I guess it's already here because scope() is always descriptor_->scope?
> 
> Another option would be to call this function something like
> GetScopeForParameterContainingEval() and have it return the scope.

I prefer the current version. It's okay with me if you don't add the DCHECK.

[adamk, 2017-02-01 16:53:22]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2017-02-01 16:53:24]

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, 2017-02-01 16:55:22]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485968002543500,
"parent_rev": "dd51dd926eff96e23b3022998f5a486f29860fac", "commit_rev":
"59b8496c81eda6da774af2991a8cf641797a754e"}

[commit-bot: I haz the power, 2017-02-01 16:55:26]

Description was changed from

==========
[parser] Remove hoist_scope from DeclarationDescriptor

The hoist_scope member of DeclarationDescriptor was only used to pass the
function
scope for declaration of parameters containing sloppy evals, for example:

  function f(x = eval("var y")) { }

In cases like this, "x" is declared in the function scope but "y" is declared in
an inner scope.
Rather than passing the function scope as "hoist_scope", we simply ask for the
outer_scope()
of the inner scope as needed in PatternRewriter.

This reduces the cognitive overhead of understanding what a
DeclarationDescriptor has; for
example, it removes some dead code from the PreParser which never has to deal
with a situation like the example above.
==========

to

==========
[parser] Remove hoist_scope from DeclarationDescriptor

The hoist_scope member of DeclarationDescriptor was only used to pass the
function
scope for declaration of parameters containing sloppy evals, for example:

  function f(x = eval("var y")) { }

In cases like this, "x" is declared in the function scope but "y" is declared in
an inner scope.
Rather than passing the function scope as "hoist_scope", we simply ask for the
outer_scope()
of the inner scope as needed in PatternRewriter.

This reduces the cognitive overhead of understanding what a
DeclarationDescriptor has; for
example, it removes some dead code from the PreParser which never has to deal
with a situation like the example above.

Review-Url: https://codereview.chromium.org/2662183002
Cr-Commit-Position: refs/heads/master@{#42861}
Committed:
https://chromium.googlesource.com/v8/v8/+/59b8496c81eda6da774af2991a8cf641797...
==========

[commit-bot: I haz the power, 2017-02-01 16:55:27]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/59b8496c81eda6da774af2991a8cf641797...