Support named arguments for dynamic calls.

Support named arguments for dynamic calls.
Committed: https://code.google.com/p/dart/source/detail?r=3786

[kasperl, 2012-02-01 10:21:21]

LGTM.

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/emitter.dart
File frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/emitter.dar...
frog/leg/emitter.dart:54: // The parameters that this stub takes.
Maybe you could write a comment with a few examples that illustrate how this
works?

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:890: // Visit positional arguments and add them to the
list.
I would factor this out into a couple of helper functions. Maybe do that in
another CL.

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.dart
File frog/leg/universe.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.da...
frog/leg/universe.dart:138: assert(other.length == 0);
Add a comment that explains that it's okay to ignore other because it is empty?
Change other.length == 0 to other.isEmpty().

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.da...
frog/leg/universe.dart:152: : super(SelectorKind.INVOCATION, argumentCount),
4 space indent?

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.da...
frog/leg/universe.dart:161: return buffer.toString();
I might be inclined to remove the buffer.add(argumentCount) call and replace the
return line with:
  
  return "${argumentCount}${buffer}";

[ngeoffray, 2012-02-01 11:44:15]

Thank you Kasper!

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/emitter.dart
File frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/emitter.dar...
frog/leg/emitter.dart:54: // The parameters that this stub takes.
On 2012/02/01 10:21:21, kasperl wrote:
> Maybe you could write a comment with a few examples that illustrate how this
> works?

Done.

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.dart
File frog/leg/universe.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.da...
frog/leg/universe.dart:138: assert(other.length == 0);
On 2012/02/01 10:21:21, kasperl wrote:
> Add a comment that explains that it's okay to ignore other because it is
empty?
> Change other.length == 0 to other.isEmpty().

Done.

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.da...
frog/leg/universe.dart:152: : super(SelectorKind.INVOCATION, argumentCount),
On 2012/02/01 10:21:21, kasperl wrote:
> 4 space indent?

Done.

https://chromiumcodereview.appspot.com/9316026/diff/4001/frog/leg/universe.da...
frog/leg/universe.dart:161: return buffer.toString();
On 2012/02/01 10:21:21, kasperl wrote:
> I might be inclined to remove the buffer.add(argumentCount) call and replace
the
> return line with:
>   
>   return "${argumentCount}${buffer}";

Done.

[floitsch, 2012-02-01 12:19:16]

LGTM.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dart
File frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:46: if (positionalArgumentCount ==
parameters.parameterCount) return;
assert that invocation.namedArgumentCount == 0 before returning?

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:63: // We fullfil the string buffers depending on the
selector. For
s/fullfil/fill?

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:78: // (3) foo$3$c(1, 2, c: 3);
remove c:

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:81: // (6) foo$4$c$d(1, 2, d: 4, c: 3);
remove d: and c: and reorder 4 and 3.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:97: if (count != 0) parametersBuffer.add(', ');
Given these annoyances with the string-buffer, maybe switch to a list<string>
and finish with join(', ').

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:98: parametersBuffer.add('${element.name}');
parameter names might need to be mangled (eg. 'with'):
JsNames.getValid(...).
At the same time please add a comment to getValid to make clear that it is an
injection/bijection. Maybe even change the name. Alternatively it would probably
be a good idea to go through to the namer.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:121: if (positionalArgumentCount != 0)
parametersBuffer.add(', ');
Only the first name in names should not have a ','.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/namer.dart
File frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/namer.dart#...
frog/leg/namer.dart:40: String instanceMethodName(SourceString name, int arity)
{
Do we still need this one?

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:918: selector.orderedNamedArguments = keys;
Why/how is keys ordered?

[ngeoffray, 2012-02-01 13:02:39]

Thanks for the comments Florian!

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dart
File frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:46: if (positionalArgumentCount ==
parameters.parameterCount) return;
On 2012/02/01 12:19:17, floitsch wrote:
> assert that invocation.namedArgumentCount == 0 before returning?

Done.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:63: // We fullfil the string buffers depending on the
selector. For
On 2012/02/01 12:19:17, floitsch wrote:
> s/fullfil/fill?

Done.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:78: // (3) foo$3$c(1, 2, c: 3);
On 2012/02/01 12:19:17, floitsch wrote:
> remove c:

Done.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:81: // (6) foo$4$c$d(1, 2, d: 4, c: 3);
On 2012/02/01 12:19:17, floitsch wrote:
> remove d: and c: and reorder 4 and 3.

Done.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:97: if (count != 0) parametersBuffer.add(', ');
On 2012/02/01 12:19:17, floitsch wrote:
> Given these annoyances with the string-buffer, maybe switch to a list<string>
> and finish with join(', '). 

Done.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:98: parametersBuffer.add('${element.name}');
On 2012/02/01 12:19:17, floitsch wrote:
> parameter names might need to be mangled (eg. 'with'):
> JsNames.getValid(...).
> At the same time please add a comment to getValid to make clear that it is an
> injection/bijection. Maybe even change the name. Alternatively it would
probably
> be a good idea to go through to the namer.

Done.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/emitter.dar...
frog/leg/emitter.dart:121: if (positionalArgumentCount != 0)
parametersBuffer.add(', ');
On 2012/02/01 12:19:17, floitsch wrote:
> Only the first name in names should not have a ','.

Gone with Strings.join.

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/namer.dart
File frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/namer.dart#...
frog/leg/namer.dart:40: String instanceMethodName(SourceString name, int arity)
{
On 2012/02/01 12:19:17, floitsch wrote:
> Do we still need this one?

For the definition of the method (that does not have a selector but just an
arity and a name).

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9316026/diff/2003/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:918: selector.orderedNamedArguments = keys;
On 2012/02/01 12:19:17, floitsch wrote:
> Why/how is keys ordered?

As discussed, ordered it in another way.