Runtime support for the new parameter specification.

Runtime support for the new parameter specification.
Committed: https://code.google.com/p/dart/source/detail?r=12258

[kasperl, 2012-09-12 07:20:30]

Maybe I can convince you to merge from trunk and re-upload?

http://codereview.chromium.org/10911211/diff/1/lib/compiler/implementation/co...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10911211/diff/1/lib/compiler/implementation/co...
lib/compiler/implementation/compiler.dart:95: static final bool
REJECT_NAMED_ARGUMENT_AS_POSITIONAL = true;
I guess this needs to be submitted as false?

[ngeoffray, 2012-09-12 07:30:08]

Uploaded to TOT. PTAL

http://codereview.chromium.org/10911211/diff/1/lib/compiler/implementation/co...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10911211/diff/1/lib/compiler/implementation/co...
lib/compiler/implementation/compiler.dart:95: static final bool
REJECT_NAMED_ARGUMENT_AS_POSITIONAL = true;
On 2012/09/12 07:20:30, kasperl wrote:
> I guess this needs to be submitted as false?

Yes :) Done

[kasperl, 2012-09-12 08:23:13]

If I'm right, I guess you need another test case.

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
File lib/compiler/implementation/universe/universe.dart (right):

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/universe/universe.dart:222: return
namedArguments.isEmpty();
Somehow this case looks too simple. If the optional parameters are positional
then we allow cases where the positionalArgumentCount is higher than the
optionalParameterCount as long as there are no named arguments?

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/universe/universe.dart:226: if (namedArgumentCount >
optionalParameterCount) return false;
Where do we weed out passing the same name more than once? I guess this check
depends on that. Probably fine.

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/universe/universe.dart:381: int foundIndex = -1;
namedArguments.indexOf(element.name)?

[ngeoffray, 2012-09-12 09:14:34]

Thanks Kasper, PTAL

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
File lib/compiler/implementation/universe/universe.dart (right):

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/universe/universe.dart:222: return
namedArguments.isEmpty();
On 2012/09/12 08:23:13, kasperl wrote:
> Somehow this case looks too simple. If the optional parameters are positional
> then we allow cases where the positionalArgumentCount is higher than the
> optionalParameterCount as long as there are no named arguments?

It is that simple :) As discussed, I added a comment. The check line 213 covers
it.

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/universe/universe.dart:226: if (namedArgumentCount >
optionalParameterCount) return false;
On 2012/09/12 08:23:13, kasperl wrote:
> Where do we weed out passing the same name more than once? I guess this check
> depends on that. Probably fine.

The check must be done in the resolver, but we're currently doing it here line
233. Added a TODO.

http://codereview.chromium.org/10911211/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/universe/universe.dart:381: int foundIndex = -1;
On 2012/09/12 08:23:13, kasperl wrote:
> namedArguments.indexOf(element.name)?

Done.

[kasperl, 2012-09-12 09:15:55]

LGTM.

[ngeoffray, 2012-09-12 11:22:38]

Thanks Kasper, but PTAL, I had to also update the names of instance methods in
the presence of the new named parameter syntax.

[kasperl, 2012-09-12 11:34:48]

LGTM.

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:352: if
(parameters.optionalParametersAreNamed
Add a comment that explains what this does.

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
File lib/compiler/implementation/js_backend/namer.dart (right):

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
lib/compiler/implementation/js_backend/namer.dart:114: StringBuffer buffer = new
StringBuffer(methodName);
I'd leave out the methodName here in the constructor call and rename buffer to
suffix. Then you can return "$methodName$suffix".

[ngeoffray, 2012-09-12 11:40:05]

Thanks Kasper

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:352: if
(parameters.optionalParametersAreNamed
On 2012/09/12 11:34:48, kasperl wrote:
> Add a comment that explains what this does.

Done.

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
File lib/compiler/implementation/js_backend/namer.dart (right):

https://chromiumcodereview.appspot.com/10911211/diff/13002/lib/compiler/imple...
lib/compiler/implementation/js_backend/namer.dart:114: StringBuffer buffer = new
StringBuffer(methodName);
On 2012/09/12 11:34:48, kasperl wrote:
> I'd leave out the methodName here in the constructor call and rename buffer to
> suffix. Then you can return "$methodName$suffix".

Done.

[ahe, 2012-09-13 14:29:10]

LGTM!

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/js_backend/emitter.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/emitter.dart:354: // If the selector has
the same number of named arguments than
than -> as

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/emitter.dart:374: // the deprecated
parameter specification.
Actually, I think this comment should be moved to addParameterStubs (plural).

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/js_backend/namer.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/namer.dart:124: return
'${name.slowToString()}\$$arity';
assert(!name.isPrivate());

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/js_backend/native_emitter.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/native_emitter.dart:415: String
toStringName = backend.namer.instanceMethodNameByArity(
Maybe rename to publicInstanceMethodByArity?

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/universe/universe.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:230: if
(positionalArgumentCount > requiredParameterCount) return false;
assert(positionalArgumentCount == requiredParameterCount);

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:238: // TODO(ngeoffray): By
removing from the set we are checking
Perhaps Jamie would like to look at that. File a bug.

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:357: if
(!Compiler.REJECT_NAMED_ARGUMENT_AS_POSITIONAL) {
TODO?

[ngeoffray, 2012-09-17 12:21:01]

Thanks Peter for the comments. Addressed in:
https://codereview.chromium.org/10919316/.

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/js_backend/emitter.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/emitter.dart:354: // If the selector has
the same number of named arguments than
On 2012/09/13 14:29:10, ahe wrote:
> than -> as

Done.

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/emitter.dart:374: // the deprecated
parameter specification.
On 2012/09/13 14:29:10, ahe wrote:
> Actually, I think this comment should be moved to addParameterStubs (plural).

Done.

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/js_backend/namer.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/namer.dart:124: return
'${name.slowToString()}\$$arity';
On 2012/09/13 14:29:10, ahe wrote:
> assert(!name.isPrivate());

Done.

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/js_backend/native_emitter.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/js_backend/native_emitter.dart:415: String
toStringName = backend.namer.instanceMethodNameByArity(
On 2012/09/13 14:29:10, ahe wrote:
> Maybe rename to publicInstanceMethodByArity?

Done.

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
File lib/compiler/implementation/universe/universe.dart (right):

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:230: if
(positionalArgumentCount > requiredParameterCount) return false;
On 2012/09/13 14:29:10, ahe wrote:
> assert(positionalArgumentCount == requiredParameterCount);

Done.

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:238: // TODO(ngeoffray): By
removing from the set we are checking
On 2012/09/13 14:29:10, ahe wrote:
> Perhaps Jamie would like to look at that. File a bug.

Done: http://code.google.com/p/dart/issues/detail?id=5213

http://codereview.chromium.org/10911211/diff/13003/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:357: if
(!Compiler.REJECT_NAMED_ARGUMENT_AS_POSITIONAL) {
On 2012/09/13 14:29:10, ahe wrote:
> TODO?

Done.