Normalization of patch signatures.

Normalization of patch signatures.

This is the second part of the patch refactoring which will require identical signatures on patches and their origins.

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

[Mads Ager (google), 2012-09-17 05:52:56]

https://codereview.chromium.org/10911325/diff/10/lib/compiler/implementation/...
File lib/compiler/implementation/lib/isolate_patch.dart (right):

https://codereview.chromium.org/10911325/diff/10/lib/compiler/implementation/...
lib/compiler/implementation/lib/isolate_patch.dart:97: patch ReceivePort get
port() {
Can this go the other way instead? That is, can we remove the '()' in
lib/isolate/ instead?

https://codereview.chromium.org/10911325/diff/10/lib/core/date.dart
File lib/core/date.dart (right):

https://codereview.chromium.org/10911325/diff/10/lib/core/date.dart#newcode51
lib/core/date.dart:51: [int month = 1,
Are default values allowed in interfaces now? They used to not be. Do you need
to change the interface here? This is not what is being patched.
DateImplementation is what is being patched (at this point).

https://codereview.chromium.org/10911325/diff/10/lib/core/regexp.dart
File lib/core/regexp.dart (right):

https://codereview.chromium.org/10911325/diff/10/lib/core/regexp.dart#newcode92
lib/core/regexp.dart:92: const RegExp(String pattern, [bool multiLine = false,
Ditto.

[ahe, 2012-09-17 07:35:04]

LGTM, provided you address all the comments.

https://codereview.chromium.org/10911325/diff/10/lib/coreimpl/list.dart
File lib/coreimpl/list.dart (right):

https://codereview.chromium.org/10911325/diff/10/lib/coreimpl/list.dart#newco...
lib/coreimpl/list.dart:30: external static List _from(Iterable<E> other);
<E> is wrong here. It is just a bogus type annotation.

[Anders Johnsen, 2012-09-17 07:53:07]

DBC

https://codereview.chromium.org/10911325/diff/10/lib/coreimpl/list.dart
File lib/coreimpl/list.dart (right):

https://codereview.chromium.org/10911325/diff/10/lib/coreimpl/list.dart#newco...
lib/coreimpl/list.dart:30: external static List _from(Iterable<E> other);
On 2012/09/17 07:35:04, ahe wrote:
> <E> is wrong here. It is just a bogus type annotation.

Ivan fixed http://code.google.com/p/dart/issues/detail?id=4727, do this should
be changed to just patch the factory.

[Lasse Reichstein Nielsen, 2012-09-17 11:32:43]

LGTM

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/compiler/implemen...
File lib/compiler/implementation/lib/isolate_patch.dart (right):

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/compiler/implemen...
lib/compiler/implementation/lib/isolate_patch.dart:97: patch ReceivePort get
port() {
I think we can, and should.

Remove ALL the getter parens!

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/core/date.dart
File lib/core/date.dart (right):

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/core/date.dart#ne...
lib/core/date.dart:51: [int month = 1,
It's allowed since v. 0.09.
But it's true that it's not related to patching.

I like the default values. We will change the interface to an abstract class
Real Soon Now, and probably to a normal factory constructor to begin with. I.e.,
personally I don't mind that this is changed, even if it's not needed.

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/coreimpl/list.dart
File lib/coreimpl/list.dart (right):

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/coreimpl/list.dar...
lib/coreimpl/list.dart:30: external static List _from(Iterable<E> other);
Yes, please!

[Mads Ager (google), 2012-09-17 11:37:26]

LGTM

Johnni, if you prefer, I can remove the parenthesis in lib/isolate after you
submit this patch. That probably requires changes in the VM patch file as well.

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/core/date.dart
File lib/core/date.dart (right):

https://chromiumcodereview.appspot.com/10911325/diff/10/lib/core/date.dart#ne...
lib/core/date.dart:51: [int month = 1,
On 2012/09/17 11:32:43, Lasse Reichstein Nielsen wrote:
> It's allowed since v. 0.09.
> But it's true that it's not related to patching.
> 
> I like the default values. We will change the interface to an abstract class
> Real Soon Now, and probably to a normal factory constructor to begin with.
I.e.,
> personally I don't mind that this is changed, even if it's not needed.

Great, now that it is allowed I'm actually strongly in favor of this change. Not
having the default values in the interface is a mess. Thanks Lasse.

[Johnni Winther, 2012-09-19 09:03:30]

http://codereview.chromium.org/10911325/diff/10/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/isolate_patch.dart (right):

http://codereview.chromium.org/10911325/diff/10/lib/compiler/implementation/l...
lib/compiler/implementation/lib/isolate_patch.dart:97: patch ReceivePort get
port() {
On 2012/09/17 05:52:56, Mads Ager wrote:
> Can this go the other way instead? That is, can we remove the '()' in
> lib/isolate/ instead?

I'll do that after the patch refactoring.

http://codereview.chromium.org/10911325/diff/10/lib/core/date.dart
File lib/core/date.dart (right):

http://codereview.chromium.org/10911325/diff/10/lib/core/date.dart#newcode51
lib/core/date.dart:51: [int month = 1,
On 2012/09/17 05:52:56, Mads Ager wrote:
> Are default values allowed in interfaces now? They used to not be. Do you need
> to change the interface here? This is not what is being patched.
> DateImplementation is what is being patched (at this point).

It seems to be allowed now and the TODO above wants it too. But you are right,
it is only needed in DateImplementation.

http://codereview.chromium.org/10911325/diff/10/lib/core/regexp.dart
File lib/core/regexp.dart (right):

http://codereview.chromium.org/10911325/diff/10/lib/core/regexp.dart#newcode92
lib/core/regexp.dart:92: const RegExp(String pattern, [bool multiLine = false,
On 2012/09/17 05:52:56, Mads Ager wrote:
> Ditto.

I especially want defaults on bool values.

http://codereview.chromium.org/10911325/diff/10/lib/coreimpl/list.dart
File lib/coreimpl/list.dart (right):

http://codereview.chromium.org/10911325/diff/10/lib/coreimpl/list.dart#newcode30
lib/coreimpl/list.dart:30: external static List _from(Iterable<E> other);
On 2012/09/17 07:53:07, ajohnsen wrote:
> On 2012/09/17 07:35:04, ahe wrote:
> > <E> is wrong here. It is just a bogus type annotation.
> 
> Ivan fixed http://code.google.com/p/dart/issues/detail?id=4727, do this should
> be changed to just patch the factory.

I guess I'll let you do that. Not really into writing patches.

[ahe, 2012-09-19 09:39:12]

http://codereview.chromium.org/10911325/diff/10/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/isolate_patch.dart (right):

http://codereview.chromium.org/10911325/diff/10/lib/compiler/implementation/l...
lib/compiler/implementation/lib/isolate_patch.dart:97: patch ReceivePort get
port() {
On 2012/09/19 09:03:30, Johnni Winther wrote:
> On 2012/09/17 05:52:56, Mads Ager wrote:
> > Can this go the other way instead? That is, can we remove the '()' in
> > lib/isolate/ instead?
> 
> I'll do that after the patch refactoring.

I'm sorry, I don't think that is the right order. By adding these parentheses,
you're regressing on the work Kasper has been doing to remove parentheses.