Update function type checking in VM after spec change (issue 444):

Update function type checking in VM after spec change (issue 444):
- the return types must be covariant or contravariant (an earlier version of the
  guide specified contravariance only).
- the names of optional parameters must match in the same order (an earlier
  version of the spec specified that any subset was allowed, no order required).
Committed: https://code.google.com/p/dart/source/detail?r=1898

[srdjan, 2011-11-29 01:01:59]

LGTM with comments

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

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.cc#newcode2933
runtime/vm/object.cc:2933: if (ParameterNameAt(i) != other.ParameterNameAt(i)) {
This is tricky, you are holding onto raw pointers for too long. I'd advise
storing both into handles and then comparing they raw pointers.

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.cc#newcode2946
runtime/vm/object.cc:2946: bool check_names) const {
I think this method has too many arguments. I suggest splitting it in 3,
something like:

CheckParameterAt(parameter_ix, type_arguments,  other_type_arguments);
CheckParameters(type_arguments,  other_type_arguments);
CheckNamedParameters(type_arguments, other_type_arguments,
num_opt_params_to_checks);

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.cc#newcode2950
runtime/vm/object.cc:2950: if (check_names && (ParameterNameAt(i) !=
other.ParameterNameAt(i))) {
ditto about holding onto raw objects.

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.h#newcode1341
runtime/vm/object.h:1341: // function type.
Add comment about from/to_parameter

[regis, 2011-11-29 01:48:21]

Thanks!

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

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.cc#newcode2933
runtime/vm/object.cc:2933: if (ParameterNameAt(i) != other.ParameterNameAt(i)) {
On 2011/11/29 01:01:59, srdjan wrote:
> This is tricky, you are holding onto raw pointers for too long. I'd advise
> storing both into handles and then comparing they raw pointers.

Done.

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.cc#newcode2946
runtime/vm/object.cc:2946: bool check_names) const {
On 2011/11/29 01:01:59, srdjan wrote:
> I think this method has too many arguments. I suggest splitting it in 3,
> something like:
> 
> CheckParameterAt(parameter_ix, type_arguments,  other_type_arguments);
> CheckParameters(type_arguments,  other_type_arguments);
> CheckNamedParameters(type_arguments, other_type_arguments,
> num_opt_params_to_checks);

I simplified the code by keeping the loop in TestType and by calling
TestParameterType on each checked parameter.

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.cc#newcode2950
runtime/vm/object.cc:2950: if (check_names && (ParameterNameAt(i) !=
other.ParameterNameAt(i))) {
On 2011/11/29 01:01:59, srdjan wrote:
> ditto about holding onto raw objects.

Done.

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/8664015/diff/1/runtime/vm/object.h#newcode1341
runtime/vm/object.h:1341: // function type.
On 2011/11/29 01:01:59, srdjan wrote:
> Add comment about from/to_parameter

Function changed.

[ahe, 2011-11-29 17:38:48]

DBC: just a minor nit in the description. The guide said that the return type is
covariant, not contravariant.

[regis, 2011-11-29 17:44:28]

On 2011/11/29 17:38:48, ahe wrote:
> DBC: just a minor nit in the description. The guide said that the return type
is
> covariant, not contravariant.

Indeed. I stand corrected.
Thanks.