closure calls (just the invocation part).

closure calls (just the invocation part).

Committed: https://code.google.com/p/dart/source/detail?r=3003

[kasperl, 2012-01-05 07:01:52]

DBC:

http://codereview.chromium.org/9086010/diff/4002/frog/leg/leg.dart
File frog/leg/leg.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/leg.dart#newcode28
frog/leg/leg.dart:28: void unreachable([msg = "UNREACHABLE"]) {
msg -> message

http://codereview.chromium.org/9086010/diff/4002/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:318: if (selector === null) {
Could we have a tester on node.selector instead? Something like isClosureCall().
Then you wouldn't need the // Closure call. comment.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:851: final DYNAMIC = 0;
How about adding a few helper methods for some of this code? This is too massive
and it makes it hard to understand what's really going on. A helper for
computing the callKind and maybe a helpers for the individual call-kind cases
(so the two switches become trivial).

http://codereview.chromium.org/9086010/diff/4002/frog/leg/typechecker.dart
File frog/leg/typechecker.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/typechecker.dart#ne...
frog/leg/typechecker.dart:311: if (selector === null) {
Use node.selector.isClosureCall or isClosureInvocation.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/typechecker.dart#ne...
frog/leg/typechecker.dart:313: // TODO(karlklose): Finish implementation;
; -> .

[floitsch, 2012-01-05 12:18:38]

PTAL.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/leg.dart
File frog/leg/leg.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/leg.dart#newcode28
frog/leg/leg.dart:28: void unreachable([msg = "UNREACHABLE"]) {
On 2012/01/05 07:01:52, kasperl wrote:
> msg -> message

Done.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:318: if (selector === null) {
On 2012/01/05 07:01:52, kasperl wrote:
> Could we have a tester on node.selector instead? Something like
isClosureCall().
> Then you wouldn't need the // Closure call. comment.

done. except in the resolver since the test needs an element.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:851: final DYNAMIC = 0;
On 2012/01/05 07:01:52, kasperl wrote:
> How about adding a few helper methods for some of this code? This is too
massive
> and it makes it hard to understand what's really going on. A helper for
> computing the callKind and maybe a helpers for the individual call-kind cases
> (so the two switches become trivial).

Done.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/typechecker.dart
File frog/leg/typechecker.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/typechecker.dart#ne...
frog/leg/typechecker.dart:311: if (selector === null) {
On 2012/01/05 07:01:52, kasperl wrote:
> Use node.selector.isClosureCall or isClosureInvocation.

Done.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/typechecker.dart#ne...
frog/leg/typechecker.dart:313: // TODO(karlklose): Finish implementation;
On 2012/01/05 07:01:52, kasperl wrote:
> ; -> .

Done.

[floitsch, 2012-01-05 12:18:38]

PTAL.

[kasperl, 2012-01-05 12:52:19]

http://codereview.chromium.org/9086010/diff/10001/frog/leg/elements/elements....
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9086010/diff/10001/frog/leg/elements/elements....
frog/leg/elements/elements.dart:367: if (element.kind === ElementKind.VARIABLE
||
Should we have element.isVariable() and element.isParameter()? Maybe just return
isVar || isParam?

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:941: /*} else if (Elements.isClosureSend(node,
elements)) {
Hmm. What's your plan here?

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:951: if (!isClosure) print(1);
Debug printing.

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:956: if (isClosure) print(2);
Debug printing.

[floitsch, 2012-01-05 13:59:19]

http://codereview.chromium.org/9086010/diff/10001/frog/leg/elements/elements....
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9086010/diff/10001/frog/leg/elements/elements....
frog/leg/elements/elements.dart:367: if (element.kind === ElementKind.VARIABLE
||
On 2012/01/05 12:52:19, kasperl wrote:
> Should we have element.isVariable() and element.isParameter()? Maybe just
return
> isVar || isParam?

Done.

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:941: /*} else if (Elements.isClosureSend(node,
elements)) {
On 2012/01/05 12:52:19, kasperl wrote:
> Hmm. What's your plan here?

oops :)
done.

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:951: if (!isClosure) print(1);
On 2012/01/05 12:52:19, kasperl wrote:
> Debug printing.

Done.

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:956: if (isClosure) print(2);
On 2012/01/05 12:52:19, kasperl wrote:
> Debug printing.

Done.

[ngeoffray, 2012-01-05 14:05:46]

LGTM

http://codereview.chromium.org/9086010/diff/4002/frog/leg/leg.dart
File frog/leg/leg.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/leg.dart#newcode28
frog/leg/leg.dart:28: void unreachable([msg = "UNREACHABLE"]) {
Since we'll get rid of this method, I suggest you don't change it, but instead
use the compiler error reporting methods. The other uses of unreachable can stay
around for now, but we need to change them.

http://codereview.chromium.org/9086010/diff/4002/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9086010/diff/4002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:319: // Closure call.
Closure call -> We're calling a closure returned from an expression.

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9086010/diff/10001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:823: visitOperatorSend(node) {
node -> Send node

http://codereview.chromium.org/9086010/diff/13001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9086010/diff/13001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:894: if (element === null) {
Why not removing that check and just do visit(node.selector); closureTarget =
pop(); ?

http://codereview.chromium.org/9086010/diff/13001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:942: assert(node.receiver === null);
I would not put that assert here. I you really want it, then I'd put it in
'isClosureSend'.

http://codereview.chromium.org/9086010/diff/13001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:955: } else {
else if (!element.isInstanceMember()) {
 ...
} else {
  compiler.internalError("Cannot generate code for send", node: node);
}
}

http://codereview.chromium.org/9086010/diff/13001/frog/leg/typechecker.dart
File frog/leg/typechecker.dart (right):

http://codereview.chromium.org/9086010/diff/13001/frog/leg/typechecker.dart#n...
frog/leg/typechecker.dart:313: print(node.selector);
Remove debugging code.

[floitsch, 2012-01-24 13:28:37]

apparently forgot to send out my response. (already more than 2 weeks old).

https://chromiumcodereview.appspot.com/9086010/diff/4002/frog/leg/leg.dart
File frog/leg/leg.dart (right):

https://chromiumcodereview.appspot.com/9086010/diff/4002/frog/leg/leg.dart#ne...
frog/leg/leg.dart:28: void unreachable([msg = "UNREACHABLE"]) {
On 2012/01/05 14:05:46, ngeoffray wrote:
> Since we'll get rid of this method, I suggest you don't change it, but instead
> use the compiler error reporting methods. The other uses of unreachable can
stay
> around for now, but we need to change them.

Done.

https://chromiumcodereview.appspot.com/9086010/diff/4002/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9086010/diff/4002/frog/leg/resolver.da...
frog/leg/resolver.dart:319: // Closure call.
On 2012/01/05 14:05:46, ngeoffray wrote:
> Closure call -> We're calling a closure returned from an expression.

Done.

https://chromiumcodereview.appspot.com/9086010/diff/13001/frog/leg/ssa/builde...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9086010/diff/13001/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:894: if (element === null) {
On 2012/01/05 14:05:46, ngeoffray wrote:
> Why not removing that check and just do visit(node.selector); closureTarget =
> pop(); ?
Because the selector doesn't (yet) have any element on it.

https://chromiumcodereview.appspot.com/9086010/diff/13001/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:942: assert(node.receiver === null);
On 2012/01/05 14:05:46, ngeoffray wrote:
> I would not put that assert here. I you really want it, then I'd put it in
> 'isClosureSend'.

moved to visitClosureSend.

https://chromiumcodereview.appspot.com/9086010/diff/13001/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:955: } else {
On 2012/01/05 14:05:46, ngeoffray wrote:
> else if (!element.isInstanceMember()) {
>  ...
> } else {
>   compiler.internalError("Cannot generate code for send", node: node);
> }
> }

Done.

https://chromiumcodereview.appspot.com/9086010/diff/13001/frog/leg/typechecke...
File frog/leg/typechecker.dart (right):

https://chromiumcodereview.appspot.com/9086010/diff/13001/frog/leg/typechecke...
frog/leg/typechecker.dart:313: print(node.selector);
On 2012/01/05 14:05:46, ngeoffray wrote:
> Remove debugging code.

Done.