[ast] Simplify FetchFreeVariables.

[ast] Simplify FetchFreeVariables.

This CL removes the ParseInfo argument from FetchFreeVariables, since it seems
to have become unnecessary.

R=verwaest@chromium.org
BUG=

Committed: https://crrev.com/7743e4993ef735e44f6ab9a2c01408ba4a1ddb28
Cr-Commit-Position: refs/heads/master@{#40933}

[Toon Verwaest, 2016-11-11 14:39:05]

lgtm

[neis, 2016-11-11 14:40:02]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-11-11 14:40:09]

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-11-11 15:09:25]

Committed patchset #1 (id:1)

[Toon Verwaest, 2016-11-11 22:14:49]

Thinking about this a little more I'm not convinced this is correct.
FetchFreeVariables destroys unresolved_ and hence subsequent allocation yields
different results than if we hadn't collected non locals. Odd that no test
failed though; but we should double-check that this doesn't break the debugger.

[neis, 2016-11-14 12:55:36]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2495293002/ by neis@chromium.org.

The reason for reverting is: It's probably needed after all but we're lacking
tests..

[neis, 2016-11-14 15:27:31]

On 2016/11/11 22:14:49, Toon Verwaest wrote:
> Thinking about this a little more I'm not convinced this is correct.
> FetchFreeVariables destroys unresolved_ and hence subsequent allocation yields
> different results than if we hadn't collected non locals. Odd that no test
> failed though; but we should double-check that this doesn't break the
debugger.

Ok, I reverted as advised.  Can you come up with a test that would fail with
this CL?  Or tell me how to write one?

Thanks.

[commit-bot: I haz the power, 2016-11-17 22:31:03]

Description was changed from

==========
[ast] Simplify FetchFreeVariables.

This CL removes the ParseInfo argument from FetchFreeVariables, since it seems
to have become unnecessary.

R=verwaest@chromium.org
BUG=
==========

to

==========
[ast] Simplify FetchFreeVariables.

This CL removes the ParseInfo argument from FetchFreeVariables, since it seems
to have become unnecessary.

R=verwaest@chromium.org
BUG=

Committed: https://crrev.com/7743e4993ef735e44f6ab9a2c01408ba4a1ddb28
Cr-Commit-Position: refs/heads/master@{#40933}
==========

[commit-bot: I haz the power, 2016-11-17 22:31:04]

Patchset 1 (id:??) landed as
https://crrev.com/7743e4993ef735e44f6ab9a2c01408ba4a1ddb28
Cr-Commit-Position: refs/heads/master@{#40933}

[adamk, 2016-11-28 20:07:43]

On 2016/11/14 15:27:31, neis wrote:
> On 2016/11/11 22:14:49, Toon Verwaest wrote:
> > Thinking about this a little more I'm not convinced this is correct.
> > FetchFreeVariables destroys unresolved_ and hence subsequent allocation
yields
> > different results than if we hadn't collected non locals. Odd that no test
> > failed though; but we should double-check that this doesn't break the
> debugger.
> 
> Ok, I reverted as advised.  Can you come up with a test that would fail with
> this CL?  Or tell me how to write one?
> 
> Thanks.

I wanted to follow up on this. Toon, you mention "subsequent allocation", but in
my reading of the code it didn't look like we ever actually do any allocation in
the debugger case (all we're trying to do with FetchFreeVariables is get a list
of names). Are there any known test cases that fail with this patch? If we can't
think of any, I feel like the code would make more sense with this re-landed...