Change core lib, dart2js, and more for new optional parameters syntax

Change core lib, dart2js, and more for new optional parameters syntax and semantics (new semantics is not enforced yet).
Committed: https://code.google.com/p/dart/source/detail?r=13684

[regis, 2012-10-09 04:26:25]

Nicolas, Kasper,

I started to change the core lib, at least so that we can run all tests. I would
like to get your opinion on the first few changes, before I continue (requires
more work), especially on the choice between optional named and optional
positional parameters.

I'll be in vacation on Tuesday.

Cheers,
Regis

[kasperl, 2012-10-09 11:49:03]

Adding Lasse (who cares a lot about our libraries) as an additional reviewer.

[Lasse Reichstein Nielsen, 2012-10-09 12:35:00]

https://codereview.chromium.org/11090016/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/coreimpl_patch.dart (right):

https://codereview.chromium.org/11090016/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/coreimpl_patch.dart:77: {int month: 1,
The implementation class should not have optional parameters.
Just make them all required, and let the factory constructor on the Date class
pass all the parameters (as it probably already does).
(In the end, I hope the implementation class goes away).

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

https://codereview.chromium.org/11090016/diff/1/lib/core/date.dart#newcode48
lib/core/date.dart:48: factory Date(int year,
Man, you just hit on our bad consciences there.
We really, really should modify the Date constructors to something useful.

Users will need a version that allows positional parameters too. I'm more
inclined to make Date that constructor, and then possibly have another (by some
name) that allows named parameters. A plain usecase is "new Date(2012,10,05)" or
"new Date(2012,10,05,14,33)". That should still be possible.

I'll loop in Florian who did the Date class.

https://codereview.chromium.org/11090016/diff/1/lib/core/date.dart#newcode56
lib/core/date.dart:56: return new DateImplementation(year, month: month, day:
day,
Modify to match requested change in DateImplementation.

https://codereview.chromium.org/11090016/diff/1/lib/coreimpl/date.dart
File lib/coreimpl/date.dart (right):

https://codereview.chromium.org/11090016/diff/1/lib/coreimpl/date.dart#newcod...
lib/coreimpl/date.dart:165: // TODO(lrn): Make parameters not optional for the
implementation class.
Do the TODO :)

https://codereview.chromium.org/11090016/diff/1/runtime/lib/date_patch.dart
File runtime/lib/date_patch.dart (right):

https://codereview.chromium.org/11090016/diff/1/runtime/lib/date_patch.dart#n...
runtime/lib/date_patch.dart:9: {int month: 1,
And again.

https://codereview.chromium.org/11090016/diff/1/tests/isolate/timer_repeat_te...
File tests/isolate/timer_repeat_test.dart (right):

https://codereview.chromium.org/11090016/diff/1/tests/isolate/timer_repeat_te...
tests/isolate/timer_repeat_test.dart:33: expectAsync1(timeoutHandler,
ITERATIONS));
Is the last parameter *ever* omitted? Otherwise, it should probably be passed
first (a small number value is best passed before a potential inline function).
If it's sometimes omitted, I liked the named version better - made it possible
to actually read it after a big handler.

[Lasse Reichstein Nielsen, 2012-10-09 12:50:40]

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

https://codereview.chromium.org/11090016/diff/1/lib/core/date.dart#newcode48
lib/core/date.dart:48: factory Date(int year,
Using optional positional parameters will make isUtc hard to use. I talked to
Florian about this, and we agree that the best solution is to have two
constructors:
 Date(int year, [int month = 1, ... ])
and
 Date.utc(int year, [int month ...])

[regis, 2012-10-11 01:33:55]

I continued working on this today. Please, have another look.
I hope to get to a point where I can submit with just a few co19 tests disabled.

Cheers,
Regis

https://codereview.chromium.org/11090016/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/coreimpl_patch.dart (right):

https://codereview.chromium.org/11090016/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/coreimpl_patch.dart:77: {int month: 1,
On 2012/10/09 12:35:00, Lasse Reichstein Nielsen wrote:
> The implementation class should not have optional parameters.
> Just make them all required, and let the factory constructor on the Date class
> pass all the parameters (as it probably already does).
> (In the end, I hope the implementation class goes away).

Done.

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

https://codereview.chromium.org/11090016/diff/1/lib/core/date.dart#newcode48
lib/core/date.dart:48: factory Date(int year,
On 2012/10/09 12:35:00, Lasse Reichstein Nielsen wrote:
> Man, you just hit on our bad consciences there.
> We really, really should modify the Date constructors to something useful.
> 
> Users will need a version that allows positional parameters too. I'm more
> inclined to make Date that constructor, and then possibly have another (by
some
> name) that allows named parameters. A plain usecase is "new Date(2012,10,05)"
or
> "new Date(2012,10,05,14,33)". That should still be possible.
> 
> I'll loop in Florian who did the Date class.

Done.

https://codereview.chromium.org/11090016/diff/1/lib/core/date.dart#newcode48
lib/core/date.dart:48: factory Date(int year,
On 2012/10/09 12:50:40, Lasse Reichstein Nielsen wrote:
> Using optional positional parameters will make isUtc hard to use. I talked to
> Florian about this, and we agree that the best solution is to have two
> constructors:
>  Date(int year, [int month = 1, ... ])
> and
>  Date.utc(int year, [int month ...])

Done.

https://codereview.chromium.org/11090016/diff/1/lib/core/date.dart#newcode56
lib/core/date.dart:56: return new DateImplementation(year, month: month, day:
day,
On 2012/10/09 12:35:00, Lasse Reichstein Nielsen wrote:
> Modify to match requested change in DateImplementation.

Done.

https://codereview.chromium.org/11090016/diff/1/lib/coreimpl/date.dart
File lib/coreimpl/date.dart (right):

https://codereview.chromium.org/11090016/diff/1/lib/coreimpl/date.dart#newcod...
lib/coreimpl/date.dart:165: // TODO(lrn): Make parameters not optional for the
implementation class.
On 2012/10/09 12:35:00, Lasse Reichstein Nielsen wrote:
> Do the TODO :)

Done.

https://codereview.chromium.org/11090016/diff/1/runtime/lib/date_patch.dart
File runtime/lib/date_patch.dart (right):

https://codereview.chromium.org/11090016/diff/1/runtime/lib/date_patch.dart#n...
runtime/lib/date_patch.dart:9: {int month: 1,
On 2012/10/09 12:35:00, Lasse Reichstein Nielsen wrote:
> And again.

Done.

https://codereview.chromium.org/11090016/diff/1/runtime/lib/date_patch.dart#n...
runtime/lib/date_patch.dart:9: {int month: 1,
On 2012/10/09 12:35:00, Lasse Reichstein Nielsen wrote:
> And again.

Done.

https://codereview.chromium.org/11090016/diff/1/tests/isolate/timer_repeat_te...
File tests/isolate/timer_repeat_test.dart (right):

https://codereview.chromium.org/11090016/diff/1/tests/isolate/timer_repeat_te...
tests/isolate/timer_repeat_test.dart:33: expectAsync1(timeoutHandler,
ITERATIONS));
On 2012/10/09 12:35:00, Lasse Reichstein Nielsen wrote:
> Is the last parameter *ever* omitted? Otherwise, it should probably be passed
> first (a small number value is best passed before a potential inline
function).
> If it's sometimes omitted, I liked the named version better - made it possible
> to actually read it after a big handler.

Yes, it is omitted a lot. I made count a named optional.

[floitsch, 2012-10-11 16:52:14]

DBC.

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1513: forEachMember(f: (_,
Element member) {
there seems to be a mismatch: here the function is named, whereas below in the
function signature the function-argument is unnamed.

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1539: void forEachMember(void
f(ClassElement enclosingClass, Element member),
I'm torn: the reason I made "f" optional was so that I could write
"includeBackendMembers: true", f: () { ... }" (with the backend-members flag
first, so that it wouldn't disappear below a big closure.
However, being forced to write "f:" all the time is not nice either.

In any case: it should probably be consistent with forEachInstanceField below.

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3316: new SsaBranchBuilder(this,
node);
does this fit on one line?

[regis, 2012-10-11 21:24:19]

Thanks!

Please, have another look and please try to approve.
This is ready for submission.

This may break some clients using the updated core lib Date API, but it will be
easy for them to fix it and we need to start fixing the code anyway.

Cheers,
Regis

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1513: forEachMember(f: (_,
Element member) {
On 2012/10/11 16:52:15, floitsch wrote:
> there seems to be a mismatch: here the function is named, whereas below in the
> function signature the function-argument is unnamed.

I guess I did not run this yet. Consistent now.

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1539: void forEachMember(void
f(ClassElement enclosingClass, Element member),
On 2012/10/11 16:52:15, floitsch wrote:
> I'm torn: the reason I made "f" optional was so that I could write
> "includeBackendMembers: true", f: () { ... }" (with the backend-members flag
> first, so that it wouldn't disappear below a big closure.
> However, being forced to write "f:" all the time is not nice either.
> 
> In any case: it should probably be consistent with forEachInstanceField below.

Yes, I have changed it back and forth too, as shown by the leftover "f:" above
:-)
Having to write "f:" everywhere is not nice, I agree. So I will leave  it as
non-optional.
I have also changed forEachInstanceField for consistency.

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11090016/diff/8001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3316: new SsaBranchBuilder(this,
node);
On 2012/10/11 16:52:15, floitsch wrote:
> does this fit on one line?

Done.

[Jennifer Messerly, 2012-10-12 02:35:58]

https://codereview.chromium.org/11090016/diff/14001/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11090016/diff/14001/pkg/dartdoc/lib/dartdoc.d...
pkg/dartdoc/lib/dartdoc.dart:1501: {MemberMirror currentMember: null,
do we need the null here?

[Lasse Reichstein Nielsen, 2012-10-12 07:52:19]

LGTM.

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

https://codereview.chromium.org/11090016/diff/14001/lib/core/date.dart#newcode46
lib/core/date.dart:46: * [:new Date(1938, 1, 10)] represents the 10th of January
1938.
Missing ':' before ']'.

https://codereview.chromium.org/11090016/diff/14001/lib/core/date.dart#newcode77
lib/core/date.dart:77: millisecond, true);
Much, much better. Thanks :)

https://codereview.chromium.org/11090016/diff/14001/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11090016/diff/14001/pkg/dartdoc/lib/dartdoc.d...
pkg/dartdoc/lib/dartdoc.dart:1501: {MemberMirror currentMember: null,
No (and we didn't before either).
I'm usually all for giving redundant information if it improves readability, but
here I'd omit the nulls too.

https://codereview.chromium.org/11090016/diff/14001/tests/lib/math/coin_test....
File tests/lib/math/coin_test.dart (right):

https://codereview.chromium.org/11090016/diff/14001/tests/lib/math/coin_test....
tests/lib/math/coin_test.dart:28: Expect.approxEquals(1.0, heads/tails, 0.1);
Unrelated comment: That looks flakey ... with ~1 flake every 2500000 runs. :)

https://codereview.chromium.org/11090016/diff/14001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/11090016/diff/14001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:228: bool recursive: false})
Do we have a rule for indentation in cases like this? I would expect the 'bool'
to be indented by one more.

[regis, 2012-10-12 17:51:51]

Thanks! I'll submit on Tuesday after triaging co19 tests again.

Cheers,
Regis

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

https://codereview.chromium.org/11090016/diff/14001/lib/core/date.dart#newcode46
lib/core/date.dart:46: * [:new Date(1938, 1, 10)] represents the 10th of January
1938.
On 2012/10/12 07:52:19, Lasse Reichstein Nielsen wrote:
> Missing ':' before ']'.

Done here and below.

https://codereview.chromium.org/11090016/diff/14001/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11090016/diff/14001/pkg/dartdoc/lib/dartdoc.d...
pkg/dartdoc/lib/dartdoc.dart:1501: {MemberMirror currentMember: null,
On 2012/10/12 07:52:19, Lasse Reichstein Nielsen wrote:
> No (and we didn't before either).
> I'm usually all for giving redundant information if it improves readability,
but
> here I'd omit the nulls too.

Lasse is correct, nulls are redundant. This cl does not try to address stylistic
issues, but since you explicitly point to these, I removed the nulls here. There
are many other instances of redundant nulls, though.

https://codereview.chromium.org/11090016/diff/14001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/11090016/diff/14001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:228: bool recursive: false})
On 2012/10/12 07:52:19, Lasse Reichstein Nielsen wrote:
> Do we have a rule for indentation in cases like this? I would expect the
'bool'
> to be indented by one more.

I am not aware of a rule. I have seen different styles. I agree that it looks
better with a space. In this particular case, we could also write both optional
parameters on the same line. We should have a clean up pass over all sources
when/if we decide that a style is better than another.