Use the typedef arity to know how to invoke a closure given by the dom.

Use the typedef arity to know how to invoke a closure given by the dom.
Committed: https://code.google.com/p/dart/source/detail?r=8067

[kasperl, 2012-05-29 12:16:24]

LGTM!

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/elements/elements.dart:372:
assert(listener.nodes.isEmpty());
Maybe cancel with an internal error instead?

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/lib/js_helper.dart:853:
convertDartClosureToJS(closure, int numberOfArguments) {
numberOfArguments -> arity?

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/native_handler.dart:220: // TODO(ngeoffray):
for static methods, we could pass a method with a
for -> For

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/resolver.dart:1137: LinkBuilder<Type> arguments
= new LinkBuilder<Type>();
Factor this code out into a helper function with a great name? You're building a
list of dynamic types that has the same length as the number of formal
parameters?

[ngeoffray, 2012-05-29 12:31:01]

Thanks Kasper

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/lib/js_helper.dart:853:
convertDartClosureToJS(closure, int numberOfArguments) {
On 2012/05/29 12:16:24, kasperl wrote:
> numberOfArguments -> arity?

Done.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/native_handler.dart:220: // TODO(ngeoffray):
for static methods, we could pass a method with a
On 2012/05/29 12:16:24, kasperl wrote:
> for -> For

Done.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/resolver.dart:1137: LinkBuilder<Type> arguments
= new LinkBuilder<Type>();
On 2012/05/29 12:16:24, kasperl wrote:
> Factor this code out into a helper function with a great name? You're building
a
> list of dynamic types that has the same length as the number of formal
> parameters?

I moved the code to TypedefElement.computeType.

[ahe, 2012-05-29 12:32:08]

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/elements/elements.dart:359: Node cachedNode;
The type should be Typedef.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/elements/elements.dart:366: Node
parseNode(Compiler compiler) {
I would prefer if this followed the "PartialFunctionElement" approach and kept
the parseNode method in the parser, that is, scanner/listener.dart.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/elements/elements.dart:372:
assert(listener.nodes.isEmpty());
On 2012/05/29 12:16:24, kasperl wrote:
> Maybe cancel with an internal error instead?

This is consistent with other code.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/resolver.dart:1133: Typedef node =
element.parseNode(compiler);
Need to wrap this in compiler.withCurrentElement.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/resolver.dart:1138: for (Link<Node> formals =
node.formals.nodes;
I'm really concerned about this approach. You need to resolve the Typedef using
the signature resolver. You cannot assume that a node is available.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/resolver.dart:1141:
arguments.addLast(compiler.types.dynamicType);
Since you only build a list of dynamicType, you don't need a LinkBuilder. The
order is irrelevant.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/typechecker.dart:109: int arity() {
This method does not have the performance characteristics that you would
naturally expect. How about calling it computeArity?

[ngeoffray, 2012-05-29 13:06:28]

Thanks Peter, PTAL

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/elements/elements.dart:359: Node cachedNode;
On 2012/05/29 12:32:08, ahe wrote:
> The type should be Typedef.

Done.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/elements/elements.dart:366: Node
parseNode(Compiler compiler) {
On 2012/05/29 12:32:08, ahe wrote:
> I would prefer if this followed the "PartialFunctionElement" approach and kept
> the parseNode method in the parser, that is, scanner/listener.dart.

Done.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/resolver.dart:1138: for (Link<Node> formals =
node.formals.nodes;
On 2012/05/29 12:32:08, ahe wrote:
> I'm really concerned about this approach. You need to resolve the Typedef
using
> the signature resolver. You cannot assume that a node is available.

Done.

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/16/dart/lib/compiler/imp...
dart/lib/compiler/implementation/typechecker.dart:109: int arity() {
On 2012/05/29 12:32:08, ahe wrote:
> This method does not have the performance characteristics that you would
> naturally expect. How about calling it computeArity?

Done.

[ahe, 2012-05-29 13:21:50]

https://chromiumcodereview.appspot.com/10441071/diff/17/dart/lib/compiler/imp...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/17/dart/lib/compiler/imp...
dart/lib/compiler/implementation/elements/elements.dart:363: Type
computeType(Compiler compiler) {
I'm absolutely 100% lukewarm on this approach. This looks like a copy of some
code I have seen in a different place.

[ngeoffray, 2012-05-29 13:43:13]

PTAL

[ahe, 2012-05-29 15:43:49]

LGTM if you address the comments.

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
File dart/lib/compiler/implementation/elements/elements.dart (left):

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/elements/elements.dart:365: position() =>
findMyName(token);
Did you mean to delete this?

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/resolver.dart:236: FunctionSignature
resolveSignature(FunctionElement element) {
Wrap in compiler.withCurrentElement

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/resolver.dart:243: FunctionSignature
resolveTypedef(TypedefElement element) {
Wrap in compiler.withCurrentElement

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
File dart/lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/scanner/listener.dart:1519: Node
parseNode(Compiler compiler) {
Try to use the parse method as the two other PartialElements do.

[ngeoffray, 2012-05-29 15:51:42]

Thanks Peter! I have also updated some status files to stop skipping on firefox.

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
File dart/lib/compiler/implementation/elements/elements.dart (left):

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/elements/elements.dart:365: position() =>
findMyName(token);
On 2012/05/29 15:43:49, ahe wrote:
> Did you mean to delete this?

Yes, it is now in PartialTypedefElement.

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/resolver.dart:236: FunctionSignature
resolveSignature(FunctionElement element) {
On 2012/05/29 15:43:49, ahe wrote:
> Wrap in compiler.withCurrentElement

Done.

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/resolver.dart:243: FunctionSignature
resolveTypedef(TypedefElement element) {
On 2012/05/29 15:43:49, ahe wrote:
> Wrap in compiler.withCurrentElement

Done.

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
File dart/lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10441071/diff/12001/dart/lib/compiler/...
dart/lib/compiler/implementation/scanner/listener.dart:1519: Node
parseNode(Compiler compiler) {
On 2012/05/29 15:43:49, ahe wrote:
> Try to use the parse method as the two other PartialElements do.

Done.