Support closures inside initializers.

Support closures inside initializers.
Committed: https://code.google.com/p/dart/source/detail?r=14333

[ngeoffray, 2012-10-30 18:32:21]

http://codereview.chromium.org/11273121/diff/4001/tests/language/language.status
File tests/language/language.status (right):

http://codereview.chromium.org/11273121/diff/4001/tests/language/language.sta...
tests/language/language.status:33: closure_in_initializer_test: Fail #
(Discussion ongoing)
Kasper, is there still discussion ongoing?

[kasperl, 2012-10-31 08:15:01]

http://codereview.chromium.org/11273121/diff/4001/tests/language/language.status
File tests/language/language.status (right):

http://codereview.chromium.org/11273121/diff/4001/tests/language/language.sta...
tests/language/language.status:33: closure_in_initializer_test: Fail #
(Discussion ongoing)
On 2012/10/30 18:32:21, ngeoffray wrote:
> Kasper, is there still discussion ongoing?

No.

[kasperl, 2012-10-31 08:25:46]

Could you add a small sample of the generated code for a simple case where you
pass a box as a parameter as a comment to this CL here?

Looks reasonably simple to me.

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/ssa/builder.dart:338: // Create a box.
Remove this comment. It's obvious from the code.

[ngeoffray, 2012-10-31 09:00:22]

For this Dart code:
class A {
  var f;
  A(a) : f = (() => a) {
    a++;
  }
}


The code generated for the factory is:

$.A$ = function(a) {
  var t1 = {}; // the box
  t1.a_1 = a;
  var t2 = new $.A(new $.anon(t1));
  t2.A$1(t1.a_1, t1); // pass the box as last parameter. Note that t1.a_1 should
not be necessary.
  return t2;
};

The closure is:
$$.anon = {"": ["box_0"],
 "super": "Closure",
 call$0: function() {
  return this.box_0.a_1;
}
};

And the class:
$$.A = {"": ["f"],
 "super": "Object",
 f$0: function() { return this.f.call$0(); },
 A$1: function(a, box_0) {
  box_0.a_1 = a; // This is harmless, but should be removed.
  box_0.a_1 = $.add(box_0.a_1, 1);
}
};






On 2012/10/31 08:25:46, kasperl wrote:
> Could you add a small sample of the generated code for a simple case where you
> pass a box as a parameter as a comment to this CL here?
> 
> Looks reasonably simple to me.
> 
>
http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
> File lib/compiler/implementation/ssa/builder.dart (right):
> 
>
http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
> lib/compiler/implementation/ssa/builder.dart:338: // Create a box.
> Remove this comment. It's obvious from the code.

[ngeoffray, 2012-10-31 09:04:28]

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/ssa/builder.dart:338: // Create a box.
On 2012/10/31 08:25:46, kasperl wrote:
> Remove this comment. It's obvious from the code.

Done.

[kasperl, 2012-10-31 09:34:28]

LGTM. Maybe file a low-priority optimization opportunity bug for the harmless,
but silly assignment?

[ngeoffray, 2012-10-31 09:38:59]

Thanks Kasper: http://code.google.com/p/dart/issues/detail?id=6421

[floitsch, 2012-10-31 10:14:50]

LGTM.

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/ssa/builder.dart:339: // TODO(floitsch): Clean up
this hack. Should we create a box-object by
please remove this comment.

http://codereview.chromium.org/11273121/diff/10002/lib/compiler/implementatio...
File lib/compiler/implementation/closure.dart (right):

http://codereview.chromium.org/11273121/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/closure.dart:48: // TODO(floitsch): we can only
assume that the reason for not having a
Remove TODO and make it an internal error.

[ngeoffray, 2012-10-31 11:28:39]

I also updated function_syntax_test which was using the bitnot operator and dart
and dart2js give different result. Since the test is more about the syntax of
function in general, I just removed the offending check.

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273121/diff/4001/lib/compiler/implementation...
lib/compiler/implementation/ssa/builder.dart:339: // TODO(floitsch): Clean up
this hack. Should we create a box-object by
On 2012/10/31 10:14:50, floitsch wrote:
> please remove this comment.

Done.

http://codereview.chromium.org/11273121/diff/10002/lib/compiler/implementatio...
File lib/compiler/implementation/closure.dart (right):

http://codereview.chromium.org/11273121/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/closure.dart:48: // TODO(floitsch): we can only
assume that the reason for not having a
On 2012/10/31 10:14:50, floitsch wrote:
> Remove TODO and make it an internal error.

Done.

[kasperl, 2012-10-31 11:29:44]

Still LGTM.

[ahe, 2012-11-06 15:14:22]

LGTM

http://codereview.chromium.org/11273121/diff/14002/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/11273121/diff/14002/lib/compiler/implementatio...
lib/compiler/implementation/ssa/codegen.dart:162: // Put the box parameter.
What does this comment mean?

http://codereview.chromium.org/11273121/diff/14002/tests/language/closure_in_...
File tests/language/closure_in_initializer2_test.dart (right):

http://codereview.chromium.org/11273121/diff/14002/tests/language/closure_in_...
tests/language/closure_in_initializer2_test.dart:4: 
What does this test do?

http://codereview.chromium.org/11273121/diff/14002/tests/language/closure_in_...
tests/language/closure_in_initializer2_test.dart:7: Expect.equals(2, this.f());
This is a static warning. Consider adding an abstract getter.

http://codereview.chromium.org/11273121/diff/14002/tests/language/closure_in_...
File tests/language/closure_in_initializer_test.dart (right):

http://codereview.chromium.org/11273121/diff/14002/tests/language/closure_in_...
tests/language/closure_in_initializer_test.dart:4: 
What does this test do?