Redirect most NewUnresolved calls over Parser

Redirect most NewUnresolved calls over Parser

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

[Toon Verwaest, 2016-08-09 19:20:02]

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

[Toon Verwaest, 2016-08-09 19:20:03]

ptal

https://codereview.chromium.org/2233473002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2233473002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:1957: return NewUnresolved(name, Variable::NORMAL,
scanner()->location().beg_pos,
Not exactly sure about redirecting this through the method above, moving
VariableMode there.

[adamk, 2016-08-09 20:11:45]

https://codereview.chromium.org/2233473002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2233473002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:279: ast_value_factory()->this_function_string(),
Variable::NORMAL, pos);
I don't think we want callers like this to have to specify Variable::NORMAL. The
only use of the Variable::Kind argument is to be able to specify Variable::THIS,
so I'd prefer if the weirdness was kept to that function alone.

https://codereview.chromium.org/2233473002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:692: return
parser_->NewUnresolved(parser_->ast_value_factory()->this_string(),
I think this is the only caller that should have to specify a Variable::Kind.

https://codereview.chromium.org/2233473002/diff/1/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2233473002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:1089: VariableMode mode = LET);
The "mode" argument on NewUnresolved makes very little sense. I wonder what
would happen if we just removed it and always added the unresolved VariableProxy
to the current scope.

[Toon Verwaest, 2016-08-10 06:45:02]

Patchset #3 (id:40001) has been deleted

[Toon Verwaest, 2016-08-10 06:47:53]

Addressed comments.

https://codereview.chromium.org/2233473002/diff/60001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2233473002/diff/60001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1967: IsLexicalVariableMode(mode) ? this->scope() :
GetDeclarationScope();
Now only declarations need to use the mode to choose the scope. Unfortunately
this is also used in the pattern rewriter with random scopes for Declare-target
and declaration-position (descriptor_->scope and descriptor_->hoist_scope).

Moving it closer to the declaration since we've anyway been discussing whether
it makes sense to have unresolved proxies for this case at all.

[adamk, 2016-08-10 17:37:43]

lgtm except for pattern-rewriter

Again, this looks like a big improvement (love the various random modes people
passed to NewUnresolved; "CONST"?)

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

https://codereview.chromium.org/2233473002/diff/60001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:153: VariableProxy* proxy =
scope->NewUnresolved(
I think this should just be unconditionally created in descriptor_->scope (and I
don't think any tests will care about the difference).

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

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:44]

The CQ bit was checked by verwaest@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2233473002/#ps80001
(title: "Addressed comment")

[commit-bot: I haz the power, 2016-08-11 11:33:53]

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:04:12]

Committed patchset #4 (id:80001)

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

Description was changed from

==========
Redirect most NewUnresolved calls over Parser

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

to

==========
Redirect most NewUnresolved calls over Parser

BUG=v8:5209

Committed: https://crrev.com/748517f7d4329c100e9eead235e1929f76cc2e35
Cr-Commit-Position: refs/heads/master@{#38575}
==========

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

Patchset 4 (id:??) landed as
https://crrev.com/748517f7d4329c100e9eead235e1929f76cc2e35
Cr-Commit-Position: refs/heads/master@{#38575}