[VM generic function types] Properly set the scope function after parsing a

[VM generic function types] Properly set the scope function after parsing a
result type that may contain a generic function type.
Add a regression test demonstrating the fixed problem.
Rename ResolveType to ResolveTypeParameters in the parser to clarify that only
type parameters get resolved.
Rename ResolveSignature to ResolveSignatureTypeParameters for the same reason.
Correctly propagate the finalization mode when finalizing signatures.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/c98989f0d7cc8c1d1520b3202e18476cc33b5691

[regis, 2017-07-18 20:23:46]

regis@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com

[rmacnak, 2017-07-18 22:06:49]

What's the size change on flutter_gallery? Do the dead functions dragged into or
the scope functions not created win the day?

[siva, 2017-07-18 23:52:25]

DBC

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
runtime/vm/clustered_snapshot.cc:1605: }
the empty if statement seems like a NOP, do we need it?

https://codereview.chromium.org/2979763002/diff/40001/tests/language/generic_...
File tests/language/generic_methods_generic_function_result_test.dart (right):

https://codereview.chromium.org/2979763002/diff/40001/tests/language/generic_...
tests/language/generic_methods_generic_function_result_test.dart:22: }
Hope this test works with dart2js, analyzer etc. otherwise you have to deal with
status files.

[regis, 2017-07-18 23:59:08]

Thanks!

I still have trouble getting the numbers Ryan asked for.

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
runtime/vm/clustered_snapshot.cc:1605: }
On 2017/07/18 23:52:25, siva wrote:
> the empty if statement seems like a NOP, do we need it?

True. We could remove the 'kind' local as well, but there is a TODO there that
we should not forget about and the dead code provides context to address this
later.
But I am happy to remove the code, up to Ryan.

https://codereview.chromium.org/2979763002/diff/40001/tests/language/generic_...
File tests/language/generic_methods_generic_function_result_test.dart (right):

https://codereview.chromium.org/2979763002/diff/40001/tests/language/generic_...
tests/language/generic_methods_generic_function_result_test.dart:22: }
On 2017/07/18 23:52:25, siva wrote:
> Hope this test works with dart2js, analyzer etc. otherwise you have to deal
with
> status files.

According to my tests, no status file change is necessary, but the bots will
have the last word.

[rmacnak, 2017-07-19 19:56:39]

lgtm

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
runtime/vm/clustered_snapshot.cc:1605: }
On 2017/07/18 23:59:07, regis wrote:
> On 2017/07/18 23:52:25, siva wrote:
> > the empty if statement seems like a NOP, do we need it?
> 
> True. We could remove the 'kind' local as well, but there is a TODO there that
> we should not forget about and the dead code provides context to address this
> later.
> But I am happy to remove the code, up to Ryan. 

Please leave the commented out code.

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7289: // For that reason, we only adjust
num_free_fun_type_params is this
if

[regis, 2017-07-19 20:14:24]

Thanks!

For the record, here are the snapshot sizes before the change:

VMIsolate(CodeSize): 4593
Isolate(CodeSize): 2369292
ReadOnlyData(CodeSize): 2125528
Instructions(CodeSize): 5227792
Total(CodeSize): 9727205

Files:
    103  gen_snapshot.d
4477504  isolate_snapshot_data
5217648  isolate_snapshot_instr
  51159  snapshot.d
  21976  vm_snapshot_data
  10144  vm_snapshot_instr


And after the change:

VMIsolate(CodeSize): 4593
Isolate(CodeSize): 2334232
ReadOnlyData(CodeSize): 2123824
Instructions(CodeSize): 5228288
Total(CodeSize): 9690937

Files:
    103 gen_snapshot.d
4440696 isolate_snapshot_data
5218144 isolate_snapshot_instr
  51159 snapshot.d
  21976 vm_snapshot_data
  10144 vm_snapshot_instr

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/clustered_sn...
runtime/vm/clustered_snapshot.cc:1605: }
On 2017/07/19 19:56:39, rmacnak wrote:
> On 2017/07/18 23:59:07, regis wrote:
> > On 2017/07/18 23:52:25, siva wrote:
> > > the empty if statement seems like a NOP, do we need it?
> > 
> > True. We could remove the 'kind' local as well, but there is a TODO there
that
> > we should not forget about and the dead code provides context to address
this
> > later.
> > But I am happy to remove the code, up to Ryan. 
> 
> Please leave the commented out code.

Acknowledged.

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2979763002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7289: // For that reason, we only adjust
num_free_fun_type_params is this
On 2017/07/19 19:56:39, rmacnak wrote:
> if

Done.

[regis, 2017-07-19 20:32:29]

Description was changed from

==========
[VM generic function types] Properly set the scope function after parsing a
result type that may contain a generic function type.
Add a regression test demonstrating the fixed problem.
Rename ResolveType to ResolveTypeParameters in the parser to clarify that only
type parameters get resolved.
Rename ResolveSignature to ResolveSignatureTypeParameters for the same reason.
Correctly propagate the finalization mode when finalizing signatures.
==========

to

==========
[VM generic function types] Properly set the scope function after parsing a
result type that may contain a generic function type.
Add a regression test demonstrating the fixed problem.
Rename ResolveType to ResolveTypeParameters in the parser to clarify that only
type parameters get resolved.
Rename ResolveSignature to ResolveSignatureTypeParameters for the same reason.
Correctly propagate the finalization mode when finalizing signatures.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c98989f0d7cc8c1d1520b3202e18476cc33b5691
==========

[regis, 2017-07-19 20:32:30]

Committed patchset #4 (id:60001) manually as
c98989f0d7cc8c1d1520b3202e18476cc33b5691 (presubmit successful).