Issue 9378040: Allow self-referencing closures.

Issue 9378040: Allow self-referencing closures. (Closed)

Created:
8 years, 10 months ago by floitsch

Modified:
8 years, 10 months ago

Reviewers:
ngeoffray

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Allow self-referencing closures. Example: var f = fib(i) { if (i < 2) return i; return fib(i - 1) + fib(i - 2); }; print(f(5)); // => 5 The only thing we need to do is make the element representing the current closure point to 'this' inside its body. In the process I had to fix some other minor issues that were triggered by this change. Committed: https://code.google.com/p/dart/source/detail?r=4183

Patch Set 1 #

Patch Set 2 : Add test. #

Patch Set 3 : Don't create a box for self-referencing closures. #

Patch Set 4 : Revert last update to CL. #

Total comments: 14

Patch Set 5 : Address comments. #

Created: 8 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+93 lines, -33 lines)			Patch
M	frog/leg/elements/elements.dart	View	1 2 3 4	2 chunks	+12 lines, -5 lines	0 comments	Download
M	frog/leg/ssa/builder.dart	View	1 2 3 4	5 chunks	+19 lines, -16 lines	0 comments	Download
M	frog/leg/ssa/closure.dart	View	1 2 3 4	6 chunks	+20 lines, -12 lines	0 comments	Download
A	frog/tests/leg_only/src/Closure4Test.dart	View	1	1 chunk	+41 lines, -0 lines	0 comments	Download
M	tests/language/language-leg.status	View		1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

ngeoffray

LGTM, with a few comments http://codereview.chromium.org/9378040/diff/13/frog/leg/elements/elements.dart File frog/leg/elements/elements.dart (right): http://codereview.chromium.org/9378040/diff/13/frog/leg/elements/elements.dart#newcode632 frog/leg/elements/elements.dart:632: element.enclosingElement.kind == ElementKind.LIBRARY) I ...

8 years, 10 months ago (2012-02-13 10:13:38 UTC) #2

floitsch

8 years, 10 months ago (2012-02-13 12:03:42 UTC) #3

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

http://codereview.chromium.org/9378040/diff/13/frog/leg/elements/elements.dar...
frog/leg/elements/elements.dart:632: element.enclosingElement.kind ==
ElementKind.LIBRARY)
On 2012/02/13 10:13:38, ngeoffray wrote:
> I believe element.enclosing is null for library elements, so you should check
> against that.
> Also, only compilation units have library as enclosing element, so I don't
think
> you need to check that enclosing is a library element.

done (null check).
refactored into a isStaticOrTopLevel

http://codereview.chromium.org/9378040/diff/13/frog/leg/elements/elements.dar...
frog/leg/elements/elements.dart:635: || element.kind === ElementKind.SETTER);
On 2012/02/13 10:13:38, ngeoffray wrote:
> Which of these kinds do not have an enclosing as compilation  unit or class?

refactored.

http://codereview.chromium.org/9378040/diff/13/frog/leg/elements/elements.dar...
frog/leg/elements/elements.dart:643: element.enclosingElement.kind ==
ElementKind.LIBRARY)
On 2012/02/13 10:13:38, ngeoffray wrote:
> Same comments as above.


the enclosing element of JS is LIBRARY.

http://codereview.chromium.org/9378040/diff/13/frog/leg/elements/elements.dar...
frog/leg/elements/elements.dart:667: if (element.isVariable() ||
element.isParameter()) return true;
On 2012/02/13 10:13:38, ngeoffray wrote:
> Could that just be return element.isLocal()?

Done.

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

http://codereview.chromium.org/9378040/diff/13/frog/leg/ssa/builder.dart#newc...
frog/leg/ssa/builder.dart:246: updateLocal(closureData.closureElement,
thisInstruction);
On 2012/02/13 10:13:38, ngeoffray wrote:
> Could you share some code with line 219? It looks like only the element to
> update is different.

Refactored together.

http://codereview.chromium.org/9378040/diff/13/frog/leg/ssa/closure.dart
File frog/leg/ssa/closure.dart (right):

http://codereview.chromium.org/9378040/diff/13/frog/leg/ssa/closure.dart#newc...
frog/leg/ssa/closure.dart:26: final FunctionElement closureElement;
On 2012/02/13 10:13:38, ngeoffray wrote:
> Should it just be null for non-closures?

Done.

http://codereview.chromium.org/9378040/diff/13/frog/leg/ssa/closure.dart#newc...
frog/leg/ssa/closure.dart:29: final ClassElement globalizedClosureElement;
On 2012/02/13 10:13:38, ngeoffray wrote:
> How about renaming it to closureClassElement?

Done.

Expand Messages | Collapse Messages