Second round of cleanups for new optional parameter semantics.

Second round of cleanups for new optional parameter semantics.
Committed: https://code.google.com/p/dart/source/detail?r=13759

[Lasse Reichstein Nielsen, 2012-10-17 14:56:10]

LGTM, but with no strong opinions about the methods being changed.

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/bidi_utils.dart
File pkg/intl/lib/bidi_utils.dart (right):

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/bidi_utils.dart#...
pkg/intl/lib/bidi_utils.dart:397: static bool detectRtlDirectionality(String
str, {bool isHtml}) {
Give it a default value of false, instead of relying on null being falsy.

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/date_symbols.dart
File pkg/intl/lib/date_symbols.dart (right):

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/date_symbols.dar...
pkg/intl/lib/date_symbols.dart:28: this.ERAS,
Indentation?
It was bad before too, but still.

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/compil...
File tests/compiler/dart2js/compiler_helper.dart (right):

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/compil...
tests/compiler/dart2js/compiler_helper.dart:22: bool minify: false}) {
It would be great if entry was an optional positional parameter, and the rest
were named. Not possible, sadly.

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/dart_b...
File tests/compiler/dart2js/dart_backend_test.dart (right):

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/dart_b...
tests/compiler/dart2js/dart_backend_test.dart:64: stripTypes: stripTypes);
This is one of the cases where we would really want to use both optional
positional and optional named parameters on the same function. It really should
be allowed.

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/redund...
File tests/compiler/dart2js/redundant_phi_eliminator_test.dart (right):

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/redund...
tests/compiler/dart2js/redundant_phi_eliminator_test.dart:34: generated =
compile(TEST_TWO, 'foo');
Missing "entry:"?

[Bob Nystrom, 2012-10-17 16:21:38]

Looked at args, unittest, and pub. Two comments, otherwise those libraries LGTM.

https://codereview.chromium.org/11194025/diff/1/pkg/unittest/test/unittest_te...
File pkg/unittest/test/unittest_test.dart (right):

https://codereview.chromium.org/11194025/diff/1/pkg/unittest/test/unittest_te...
pkg/unittest/test/unittest_test.dart:37: String message: ''}) {
Check with Gram, but instead of making these named, how about making them
optional positional parameters and moving message to be the first one?

https://codereview.chromium.org/11194025/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11194025/diff/1/utils/pub/io.dart#newcode452
utils/pub/io.dart:452: {void onClosed()}) {
Make this positional please.

[regis, 2012-10-17 19:58:15]

Thanks!

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/bidi_utils.dart
File pkg/intl/lib/bidi_utils.dart (right):

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/bidi_utils.dart#...
pkg/intl/lib/bidi_utils.dart:397: static bool detectRtlDirectionality(String
str, {bool isHtml}) {
On 2012/10/17 14:56:10, Lasse Reichstein Nielsen wrote:
> Give it a default value of false, instead of relying on null being falsy.

Done. But I need to postpone these changes in bidi_util, because of an apparent
bug in dart2js. I'll file a bug.

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/date_symbols.dart
File pkg/intl/lib/date_symbols.dart (right):

https://codereview.chromium.org/11194025/diff/1/pkg/intl/lib/date_symbols.dar...
pkg/intl/lib/date_symbols.dart:28: this.ERAS,
On 2012/10/17 14:56:10, Lasse Reichstein Nielsen wrote:
> Indentation?
> It was bad before too, but still.

Done.

https://codereview.chromium.org/11194025/diff/1/pkg/unittest/test/unittest_te...
File pkg/unittest/test/unittest_test.dart (right):

https://codereview.chromium.org/11194025/diff/1/pkg/unittest/test/unittest_te...
pkg/unittest/test/unittest_test.dart:37: String message: ''}) {
On 2012/10/17 16:21:38, Bob Nystrom wrote:
> Check with Gram, but instead of making these named, how about making them
> optional positional parameters and moving message to be the first one?

Only 3 out of 14 calls pass a message, so I am not sure this is the right thing
to do. In any case, feel free to change it later the way you want. I am more
concerned about correctness than style in this cl.

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/compil...
File tests/compiler/dart2js/compiler_helper.dart (right):

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/compil...
tests/compiler/dart2js/compiler_helper.dart:22: bool minify: false}) {
On 2012/10/17 14:56:10, Lasse Reichstein Nielsen wrote:
> It would be great if entry was an optional positional parameter, and the rest
> were named. Not possible, sadly.

Yes, a mix of both flavors would be useful here.

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/dart_b...
File tests/compiler/dart2js/dart_backend_test.dart (right):

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/dart_b...
tests/compiler/dart2js/dart_backend_test.dart:64: stripTypes: stripTypes);
On 2012/10/17 14:56:10, Lasse Reichstein Nielsen wrote:
> This is one of the cases where we would really want to use both optional
> positional and optional named parameters on the same function. It really
should
> be allowed.

Agreed.

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/redund...
File tests/compiler/dart2js/redundant_phi_eliminator_test.dart (right):

https://codereview.chromium.org/11194025/diff/1/tests/compiler/dart2js/redund...
tests/compiler/dart2js/redundant_phi_eliminator_test.dart:34: generated =
compile(TEST_TWO, 'foo');
On 2012/10/17 14:56:10, Lasse Reichstein Nielsen wrote:
> Missing "entry:"?

Done.

https://codereview.chromium.org/11194025/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11194025/diff/1/utils/pub/io.dart#newcode452
utils/pub/io.dart:452: {void onClosed()}) {
On 2012/10/17 16:21:38, Bob Nystrom wrote:
> Make this positional please.

Done.

[ahe, 2012-10-22 12:44:17]

Thank you, Regis, for cleaning up the parameter semantics!

I already have a CL to fix the dart2js crash.

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

https://codereview.chromium.org/11194025/diff/7002/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3449: <HInstruction>[oldVariable]);
This is the cause of the crash.

https://codereview.chromium.org/11194025/diff/7002/tests/co19/co19-dart2js.st...
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/11194025/diff/7002/tests/co19/co19-dart2js.st...
tests/co19/co19-dart2js.status:9: Language/11_Statements/05_For_A01_t07: Crash
We generally try to avoid submitting changes to dart2js that introduce new
crashes.