Issue 9542001: Fix this capturing in closures.

Issue 9542001: Fix this capturing in closures. (Closed)

Created:
8 years, 9 months ago by floitsch

Modified:
8 years, 9 months ago

Reviewers:
ahe, ngeoffray

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Fix this capturing in closures. Committed: https://code.google.com/p/dart/source/detail?r=4749

Patch Set 2 : Address comments. #

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+18 lines, -39 lines)			Patch
M	frog/leg/ssa/builder.dart	View	1	2 chunks	+12 lines, -5 lines	0 comments	Download
M	frog/leg/ssa/closure.dart	View		1 chunk	+6 lines, -2 lines	0 comments	Download
M	frog/tests/leg_only/leg_only.status	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	tests/co19/co19-leg.status	View	1	3 chunks	+0 lines, -29 lines	0 comments	Download
M	tests/language/language-leg.status	View	1	2 chunks	+0 lines, -2 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

ahe

LGTM, but you may want Nicolas' opinion as well. https://chromiumcodereview.appspot.com/9542001/diff/1/tests/co19/co19-leg.status File tests/co19/co19-leg.status (right): https://chromiumcodereview.appspot.com/9542001/diff/1/tests/co19/co19-leg.status#newcode53 tests/co19/co19-leg.status:53: ...

8 years, 9 months ago (2012-02-29 12:22:06 UTC) #2

ngeoffray

LGTM https://chromiumcodereview.appspot.com/9542001/diff/1/frog/leg/ssa/builder.dart File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9542001/diff/1/frog/leg/ssa/builder.dart#newcode266 frog/leg/ssa/builder.dart:266: // instance-members that do *not* have any thisElement. ...

8 years, 9 months ago (2012-02-29 12:22:17 UTC) #3

floitsch

8 years, 9 months ago (2012-02-29 12:35:27 UTC) #4

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

https://chromiumcodereview.appspot.com/9542001/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:266: // instance-members that do *not* have any
thisElement.
On 2012/02/29 12:22:17, ngeoffray wrote:
> Please explain why.

Done.

https://chromiumcodereview.appspot.com/9542001/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:273: // directLocals[closureData.thisElement] =
thisInstruction;
On 2012/02/29 12:22:17, ngeoffray wrote:
> Why is this commented out?
this one is the correct one...

https://chromiumcodereview.appspot.com/9542001/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:274: updateLocal(closureData.thisElement,
thisInstruction);
On 2012/02/29 12:22:17, ngeoffray wrote:
> Why do you call updateLocal, and not use directLocals, like for parameters
line
> 262.
debug code...

https://chromiumcodereview.appspot.com/9542001/diff/1/tests/co19/co19-leg.status
File tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9542001/diff/1/tests/co19/co19-leg.sta...
tests/co19/co19-leg.status:53: # Fail type assertion on VM, cancels otherwise.
On 2012/02/29 12:22:06, ahe wrote:
> Please remove this line.

Done.

Expand Messages | Collapse Messages