Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(745)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 3009903002: Pass in `this` as a free variable to the closure class (Closed)

Created:
3 years, 3 months ago by Emily Fortuna
Modified:
3 years, 3 months ago
Reviewers:
Johnni Winther, sra1, kevmoo
CC:
reviews_dartlang.org
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Pass in `this` as a free variable to the closure class BUG= R=johnniwinther@google.com Committed: https://github.com/dart-lang/sdk/commit/ea038bd8e099bd1a358c9e952364621705d9f902

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : dartfmt #

Patch Set 4 : . #

Total comments: 4

Patch Set 5 : merge with master #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+111 lines, -37 lines) Patch
M pkg/compiler/lib/src/closure.dart View 1 2 3 4 2 chunks +9 lines, -1 line 0 comments Download
M pkg/compiler/lib/src/js_emitter/runtime_type_generator.dart View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
M pkg/compiler/lib/src/js_model/closure.dart View 1 2 3 4 2 chunks +10 lines, -1 line 0 comments Download
M pkg/compiler/lib/src/js_model/closure_visitors.dart View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
M pkg/compiler/lib/src/kernel/element_map_impl.dart View 1 2 3 4 2 chunks +27 lines, -7 lines 0 comments Download
M tests/compiler/dart2js/closure/data/captured_variable.dart View 3 2 chunks +10 lines, -0 lines 0 comments Download
M tests/compiler/dart2js/equivalence/id_equivalence.dart View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M tests/compiler/dart2js/equivalence/id_equivalence_test.dart View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M tests/compiler/dart2js_extra/dart2js_extra.status View 1 2 3 3 chunks +2 lines, -4 lines 0 comments Download
M tests/language/language_dart2js.status View 1 2 3 4 8 chunks +4 lines, -10 lines 0 comments Download
M tests/language_2/language_2_dart2js.status View 1 2 3 4 8 chunks +6 lines, -12 lines 2 comments Download

Messages

Total messages: 12 (5 generated)
Emily Fortuna
Not a huge win in more tests passing, but a small piece.
3 years, 3 months ago (2017-08-31 00:27:24 UTC) #3
Johnni Winther
lgtm https://codereview.chromium.org/3009903002/diff/60001/pkg/compiler/lib/src/js_model/closure_visitors.dart File pkg/compiler/lib/src/js_model/closure_visitors.dart (right): https://codereview.chromium.org/3009903002/diff/60001/pkg/compiler/lib/src/js_model/closure_visitors.dart#newcode212 pkg/compiler/lib/src/js_model/closure_visitors.dart:212: _currentScopeInfo.freeVariables.add(const ThisVariable()); Add a `needsThis` property on [KernelScopeInfo] ...
3 years, 3 months ago (2017-08-31 07:24:38 UTC) #4
sra1
https://codereview.chromium.org/3009903002/diff/60001/pkg/compiler/lib/src/closure.dart File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/3009903002/diff/60001/pkg/compiler/lib/src/closure.dart#newcode627 pkg/compiler/lib/src/closure.dart:627: bool operator ==(obj) { use 'other' for consistency with ...
3 years, 3 months ago (2017-08-31 16:26:54 UTC) #5
Emily Fortuna
https://codereview.chromium.org/3009903002/diff/60001/pkg/compiler/lib/src/closure.dart File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/3009903002/diff/60001/pkg/compiler/lib/src/closure.dart#newcode627 pkg/compiler/lib/src/closure.dart:627: bool operator ==(obj) { On 2017/08/31 16:26:54, sra1 wrote: ...
3 years, 3 months ago (2017-08-31 17:40:52 UTC) #6
Emily Fortuna
Committed patchset #5 (id:100001) manually as ea038bd8e099bd1a358c9e952364621705d9f902 (presubmit successful).
3 years, 3 months ago (2017-08-31 17:41:25 UTC) #9
kevmoo
DBQ https://codereview.chromium.org/3009903002/diff/100001/tests/language_2/language_2_dart2js.status File tests/language_2/language_2_dart2js.status (right): https://codereview.chromium.org/3009903002/diff/100001/tests/language_2/language_2_dart2js.status#newcode556 tests/language_2/language_2_dart2js.status:556: generic_methods_local_variable_declaration_test: Pass This seems – weird. Shouldn't the ...
3 years, 3 months ago (2017-08-31 18:13:57 UTC) #11
Emily Fortuna
3 years, 3 months ago (2017-08-31 18:19:02 UTC) #12
Message was sent while issue was closed. 
https://codereview.chromium.org/3009903002/diff/100001/tests/language_2/langu...
File tests/language_2/language_2_dart2js.status (right):

https://codereview.chromium.org/3009903002/diff/100001/tests/language_2/langu...
tests/language_2/language_2_dart2js.status:556:
generic_methods_local_variable_declaration_test: Pass
On 2017/08/31 18:13:57, kevmoo wrote:
> This seems – weird. Shouldn't the previous method line just be deleted?

short answer -- yes, that does seem weird. I'll send a follow-up CL. 

I updated this status file with Siggi's script. I think that means there's a bug
in the script. I'll talk to Siggi about it.

Powered by Google App Engine
This is Rietveld 408576698