Preparsing inner funcs: be less pessimistic about maybe_assigned.

Preparsing inner funcs: be less pessimistic about maybe_assigned.

BUG=v8:5501, v8:5678
Committed: https://crrev.com/64d9352a545c13e1c9519137c935f3c0bcd03177
Cr-Commit-Position: refs/heads/master@{#41645}

[marja, 2016-11-30 12:59:54]

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

[marja, 2016-11-30 12:59:55]

this is the next step - ptal

This cl is based on https://codereview.chromium.org/2407163003/

[Toon Verwaest, 2016-12-01 13:34:08]

Looks good! Simple enough :)

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

https://codereview.chromium.org/2539123002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:8: #include "src/ast/ast.h"
Should we split out VariableProxy into a separate file just like Variable?

https://codereview.chromium.org/2539123002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:1251: if (expression.variables_ != nullptr) {
This is still broken I think since it's broken in the CL this is based on.

[marja, 2016-12-06 14:50:06]

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

https://codereview.chromium.org/2539123002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:8: #include "src/ast/ast.h"
On 2016/12/01 13:34:08, Toon Verwaest wrote:
> Should we split out VariableProxy into a separate file just like Variable?

It won't help here since we need to call AstNodeFactory()->NewVariableProxy().

https://codereview.chromium.org/2539123002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:1251: if (expression.variables_ != nullptr) {
On 2016/12/01 13:34:08, Toon Verwaest wrote:
> This is still broken I think since it's broken in the CL this is based on.

Hmm, what do you mean?

[Toon Verwaest, 2016-12-09 11:43:12]

lgtm

[marja, 2016-12-12 09:43:02]

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

[commit-bot: I haz the power, 2016-12-12 09:43:12]

Dry run: 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-12-12 10:08:41]

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

[commit-bot: I haz the power, 2016-12-12 10:08:42]

Dry run: This issue passed the CQ dry run.

[neis, 2016-12-12 13:01:35]

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

[neis, 2016-12-12 13:01:36]

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:160: ZoneList<VariableProxy*>* variables = nullptr) {
Can we remove the default value here and below? I think it's not used.

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:546: }
Why do we create a variable proxy for a string literal?

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:1226: // to start tracking which expressions are
variables and which are assigned.
Is this TODO now resolved?

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:1263: MarkExpressionAsAssigned(each);
Maybe reflect this change in the CL description.

https://codereview.chromium.org/2539123002/diff/80001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2539123002/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3367: };
I don't understand how you choose the allow_error_in_inner_function value for
these tests.

[marja, 2016-12-12 13:36:47]

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:160: ZoneList<VariableProxy*>* variables = nullptr) {
On 2016/12/12 13:01:36, neis wrote:
> Can we remove the default value here and below? I think it's not used.

Done.

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:546: }
On 2016/12/12 13:01:36, neis wrote:
> Why do we create a variable proxy for a string literal?

Discussed offline: for object literal properties which through this code path
(see comment above).

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:1226: // to start tracking which expressions are
variables and which are assigned.
On 2016/12/12 13:01:36, neis wrote:
> Is this TODO now resolved?

Sadly, no. It's orthogonal-ish. We still only track the variables in the
track_unresolved_variables_ = true mode, and don't track them in a way that
would allow us to produce all the errors.. (and error producing is not
implemented).

https://codereview.chromium.org/2539123002/diff/80001/src/parsing/preparser.h...
src/parsing/preparser.h:1263: MarkExpressionAsAssigned(each);
On 2016/12/12 13:01:36, neis wrote:
> Maybe reflect this change in the CL description.

Offline discussion: this falls under the description.

https://codereview.chromium.org/2539123002/diff/80001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2539123002/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3367: };
On 2016/12/12 13:01:36, neis wrote:
> I don't understand how you choose the allow_error_in_inner_function value for
> these tests.

Updated the comment above

[neis, 2016-12-12 13:39:28]

lgtm

[marja, 2016-12-12 13:42:07]

The CQ bit was checked by marja@chromium.org

[marja, 2016-12-12 13:42:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2539123002/#ps120001
(title: "moar")

[commit-bot: I haz the power, 2016-12-12 13:42:12]

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-12-12 14:44:30]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481550127946470,
"parent_rev": "de99f3ab1c758936389ea4c9f85d3f2a32b4c69b", "commit_rev":
"4e05e3c05127162fefa7408644bbff63d5d548f6"}

[commit-bot: I haz the power, 2016-12-12 14:44:35]

Description was changed from

==========
Preparsing inner funcs: be less pessimistic about maybe_assigned.

BUG=v8:5501, v8:5678
==========

to

==========
Preparsing inner funcs: be less pessimistic about maybe_assigned.

BUG=v8:5501, v8:5678

Review-Url: https://codereview.chromium.org/2539123002
==========

[commit-bot: I haz the power, 2016-12-12 14:44:35]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-12 14:45:20]

Description was changed from

==========
Preparsing inner funcs: be less pessimistic about maybe_assigned.

BUG=v8:5501, v8:5678

Review-Url: https://codereview.chromium.org/2539123002
==========

to

==========
Preparsing inner funcs: be less pessimistic about maybe_assigned.

BUG=v8:5501, v8:5678

Committed: https://crrev.com/64d9352a545c13e1c9519137c935f3c0bcd03177
Cr-Commit-Position: refs/heads/master@{#41645}
==========

[commit-bot: I haz the power, 2016-12-12 14:45:21]

Patchset 7 (id:??) landed as
https://crrev.com/64d9352a545c13e1c9519137c935f3c0bcd03177
Cr-Commit-Position: refs/heads/master@{#41645}