Process generic function type arguments in more places (function type tests,

Process generic function type arguments in more places (function type tests,
function cloning for mixins, signature building, etc...).
Rename --generic-method-semantics flag to --reify-generic-functions (still not
working and disabled by default).

R=asiva@google.com

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

[regis, 2017-04-20 19:04:20]

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

[zra, 2017-04-20 19:22:09]

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

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcode55
runtime/vm/object.cc:55: DEFINE_FLAG(bool,
Should we put this in flag_list.h?

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6905: const String& closure_name = String::Handle(name());
Maybe grab the Zone ahead of time for this function as well.

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:18151: TypeParameter& clone = TypeParameter::Handle();
ditto

[regis, 2017-04-20 20:23:30]

Thanks!

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

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcode55
runtime/vm/object.cc:55: DEFINE_FLAG(bool,
On 2017/04/20 19:22:09, zra wrote:
> Should we put this in flag_list.h?

Done.

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6905: const String& closure_name = String::Handle(name());
On 2017/04/20 19:22:09, zra wrote:
> Maybe grab the Zone ahead of time for this function as well.

Done.

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:18151: TypeParameter& clone = TypeParameter::Handle();
On 2017/04/20 19:22:09, zra wrote:
> ditto

Done.

[siva, 2017-04-20 20:32:08]

LGTM with some comments.

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

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6603: other_bound = other_type_param.bound();
Should we assert here that the two types are finalized? I am wondering about the
recursive case...

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7137: pieces.Add(Symbols::RAngleBracket());
Do we still want '<>' if num_type_params is 0? or maybe we just need to assert
above that num_type_params is not 0 as !type_params.IsNull() means we should
have at least one type parameter.

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:17222: }
This code here is very similar to the one in Function::TypeTest maybe extract
this into a helper function?

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:17326: }
ditto comment about this matching up with the one in Function::Clone and the
chunk below.

[regis, 2017-04-20 21:18:26]

Thanks!

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

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6603: other_bound = other_type_param.bound();
On 2017/04/20 20:32:08, siva wrote:
> Should we assert here that the two types are finalized? I am wondering about
the
> recursive case...

The Equals call should do the right thing in case one or the other type are not
finalized. However, we are performing a type test here, which assumes that types
are finalized. So you are right, it is better to detect a problem early. I added
asserts.

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7137: pieces.Add(Symbols::RAngleBracket());
On 2017/04/20 20:32:08, siva wrote:
> Do we still want '<>' if num_type_params is 0? or maybe we just need to assert
> above that num_type_params is not 0 as !type_params.IsNull() means we should
> have at least one type parameter.

Yes, we should never see an empty array. I added an assert.

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:17222: }
On 2017/04/20 20:32:08, siva wrote:
> This code here is very similar to the one in Function::TypeTest maybe extract
> this into a helper function?

That was bugging me too, and then I forgot :-)
I added a Function::HasSameTypeParametersAndBounds(other).

https://codereview.chromium.org/2835513002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:17326: }
On 2017/04/20 20:32:08, siva wrote:
> ditto comment about this matching up with the one in Function::Clone and the
> chunk below.

This one cannot easily be factored out. One is calling CloneUnfinalized() and
the other one CloneUninstantiated(new_owner, trail).
This is not worth it.

[regis, 2017-04-20 21:20:23]

Description was changed from

==========
Process generic function type arguments in more places (function type tests,
function cloning for mixins, signature building, etc...).
Rename --generic-method-semantics flag to --reify-generic-functions (still not
working and disabled by default).
==========

to

==========
Process generic function type arguments in more places (function type tests,
function cloning for mixins, signature building, etc...).
Rename --generic-method-semantics flag to --reify-generic-functions (still not
working and disabled by default).

R=asiva@google.com

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

[regis, 2017-04-20 21:20:24]

Committed patchset #3 (id:40001) manually as
1b68c0d0a1546313b2aebc9f833222838a3a94c2 (presubmit successful).