Patch signatures checked in resolver.

Patch signatures checked in resolver.

Committed: https://code.google.com/p/dart/source/detail?r=13285

[Lasse Reichstein Nielsen, 2012-10-04 07:51:21]

LGTM

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:143: // we are restricting patch uses
and the approach cannot be used
How are we restricting it?
And isn't this incredibly inefficient?

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:149: if (originParameterText !=
patchParameterText) {
Does this require that the positional parameters have the same name? It's
reasonable, but not really necessary.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:175: 
Why is there not a == on FunctionSignature that does all this?

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:202:
patchSignature.optionalParameters);
Does this handle both optional positional parameters and optional named
parameters?

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
File lib/compiler/implementation/warnings.dart (right):

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/warnings.dart:268: "Required parameter count of
patch method (#{3}) on origin method #{1}.");
Message doesn't say what the problem is.

Seems odd to not have a #{2}. Could you swap #2 and #3 so it's #3 that's
omitted?

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/warnings.dart:271: "Optional parameter count of
patch method (#{3}) on origin method #{1}.");
Doesn't say what the problem is. I need to know what I should change to make it
succeede.

[ahe, 2012-10-04 08:34:21]

LGTM!

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:178: originTree.returnType !== null ?
originTree.returnType : originTree;
I think the errorNode should be derived from patchTree.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:184: error(originTree,
patchTree

[Johnni Winther, 2012-10-04 10:23:29]

Tests of checks added to patch_test.
PTAL

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:143: // we are restricting patch uses
and the approach cannot be used
On 2012/10/04 07:51:21, Lasse Reichstein Nielsen wrote:
> How are we restricting it?
> And isn't this incredibly inefficient?

Restrictions: See the next comment.
Inefficient: Yes, but parameter node trees are generally small.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:149: if (originParameterText !=
patchParameterText) {
On 2012/10/04 07:51:21, Lasse Reichstein Nielsen wrote:
> Does this require that the positional parameters have the same name? It's
> reasonable, but not really necessary.

Yes, parameters must have the same name, and we do not even allow a constructor
implementation to use this.field. We can implement patches without this
flexibility, so the cost of writing a more liberal equality for parameters
seemed to high.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:175: 
On 2012/10/04 07:51:21, Lasse Reichstein Nielsen wrote:
> Why is there not a == on FunctionSignature that does all this?

It would require the aforementioned equality on parameters.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:178: originTree.returnType !== null ?
originTree.returnType : originTree;
On 2012/10/04 08:34:21, ahe wrote:
> I think the errorNode should be derived from patchTree.

Done.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:184: error(originTree,
On 2012/10/04 08:34:21, ahe wrote:
> patchTree

Done.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:202:
patchSignature.optionalParameters);
On 2012/10/04 07:51:21, Lasse Reichstein Nielsen wrote:
> Does this handle both optional positional parameters and optional named
> parameters?

Yes, but a check is added to ensure that both are either named or positional.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
File lib/compiler/implementation/warnings.dart (right):

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/warnings.dart:268: "Required parameter count of
patch method (#{3}) on origin method #{1}.");
On 2012/10/04 07:51:21, Lasse Reichstein Nielsen wrote:
> Message doesn't say what the problem is.
> 
> Seems odd to not have a #{2}. Could you swap #2 and #3 so it's #3 that's
> omitted?

Done.

https://chromiumcodereview.appspot.com/11052012/diff/1/lib/compiler/implement...
lib/compiler/implementation/warnings.dart:271: "Optional parameter count of
patch method (#{3}) on origin method #{1}.");
On 2012/10/04 07:51:21, Lasse Reichstein Nielsen wrote:
> Doesn't say what the problem is. I need to know what I should change to make
it
> succeede.

Done.

[Lasse Reichstein Nielsen, 2012-10-04 11:01:49]

LGTM

https://codereview.chromium.org/11052012/diff/6001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

https://codereview.chromium.org/11052012/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:211:
patchSignature.optionalParametersAreNamed) {
Check this before comparing the actual parameters, to give the most relevant
error.

[ahe, 2012-10-04 11:10:30]

LGTM!

[Johnni Winther, 2012-10-05 11:43:22]

https://codereview.chromium.org/11052012/diff/6001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

https://codereview.chromium.org/11052012/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:211:
patchSignature.optionalParametersAreNamed) {
On 2012/10/04 11:01:49, Lasse Reichstein Nielsen wrote:
> Check this before comparing the actual parameters, to give the most relevant
> error.

Done.