Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1450393002:
Roll sdk dependency to 34357cdad108dcba734949bd13bd28c76ea285e0 (Closed)
|
Created:
5 years, 1 month ago by sigurdm Modified:
5 years ago CC:
fletch+reviews_googlegroups.com Base URL:
git@github.com:dart-lang/fletch.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionRoll sdk dependency to 34357cdad108dcba734949bd13bd28c76ea285e0
Includes updates to fletchc to adapt to changes in the dart2js frontend.
R=ajohnsen@google.com, johnniwinther@google.com
Committed: https://github.com/dart-lang/fletch/commit/9a7963cf02e632b68cc9e936fb5879b5da7864ec
Reverted: https://github.com/dart-lang/fletch/commit/4faa68c323d39de9c20f3aa58c76e8418a8c1d85
Committed: https://github.com/dart-lang/fletch/commit/3b5c04dca91c04f05c762e5eb98ca1dd93de7ce2
Patch Set 1 : #
Total comments: 86
Patch Set 2 : Address review, simplify registry, point at later revision #Patch Set 3 : Whitespace fix #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : Don't return transformed impact when nothing changed. #Patch Set 6 : Whitespace fix #Patch Set 7 : Update binary shas and dart-rev sha #Patch Set 8 : rebase, remove dead function #Patch Set 9 : Update status files #
Total comments: 98
Patch Set 10 : Address ahe's review #
Messages
Total messages: 26 (6 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
sigurdm@google.com changed reviewers: + ahe@google.com, ajohnsen@google.com, johnniwinther@google.com
I think you need to update the Dart VM binaries as well.
Thanks for doing this work, Sigurd. I suggest that we sit down together tomorrow and work through my comments. Passing a FletchRegistry to the DebugInfo visitors is a problem we need to resolve. https://codereview.chromium.org/1450393002/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/1450393002/diff/40001/DEPS#newcode37 DEPS:37: Extra line. https://codereview.chromium.org/1450393002/diff/40001/lib/core/core_patch.dart File lib/core/core_patch.dart (right): https://codereview.chromium.org/1450393002/diff/40001/lib/core/core_patch.dar... lib/core/core_patch.dart:674: @patch static String _uriEncode( Where is this code from? Is it a copy of something? https://codereview.chromium.org/1450393002/diff/40001/lib/system/system.dart File lib/system/system.dart (right): https://codereview.chromium.org/1450393002/diff/40001/lib/system/system.dart#... lib/system/system.dart:89: class Patch { What is this class for? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/increme... File pkg/fletchc/lib/incremental/library_updater.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/increme... pkg/fletchc/lib/incremental/library_updater.dart:796: if (before.isMalformed || What does isMalformed mean? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:3180: new List(); ? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:3223: void registerDynamicUse(DynamicUse use) { This doesn't seem like the right API to me. We shouldn't be creating a bunch of DynamicUse instances just to throw them away again. That's what will happen when generating debug information. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... File pkg/fletchc/lib/src/debug_info_constructor_codegen.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... pkg/fletchc/lib/src/debug_info_constructor_codegen.dart:68: new FletchRegistry(compiler, null), This is a problem. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... File pkg/fletchc/lib/src/debug_registry.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... pkg/fletchc/lib/src/debug_registry.dart:55: return null; Throw instead? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... File pkg/fletchc/lib/src/driver/developer.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... pkg/fletchc/lib/src/driver/developer.dart:421: return null; Why this change? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... File pkg/fletchc/lib/src/driver/driver_main.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... pkg/fletchc/lib/src/driver/driver_main.dart:460: // executable (the C++ program). Why this change? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dyn... File pkg/fletchc/lib/src/dynamic_call_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dyn... pkg/fletchc/lib/src/dynamic_call_enqueuer.dart:199: void enqueueSelector(DynamicUse use) { We need to change this. Not sure how yet. The method name is wrong. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/enq... File pkg/fletchc/lib/src/enqueuer_mixin.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/enq... pkg/fletchc/lib/src/enqueuer_mixin.dart:11: class EnqueuerMixin implements CodegenEnqueuer { Not sure about this. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/enq... pkg/fletchc/lib/src/enqueuer_mixin.dart:175: @override Please don't use @override, or use it consistently. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_backend.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:60: import 'package:compiler/src/universe/use.dart' show DynamicUse, StaticUse, TypeUse, TypeUseKind; Long line. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:127: import 'package:compiler/src/universe/world_impact.dart' show TransformedWorldImpact, WorldImpact, WorldImpactBuilder; long line. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:276: Identifiers.call); Fits on one line? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:472: // void onMapLiteral(Registry registry, DartType type, bool isConstant) { Remove? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:725: new FletchRegistry(compiler, null), I think this is wrong. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:739: new FletchRegistry(compiler, null), Ditto. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:750: new FletchRegistry(compiler, null), Ditto. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:911: compiler.reporter.internalError(element, "Couldn't find tear-off stub"); Long line. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1190: new FletchRegistry(compiler, null); Fits on one line? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1320: new Selector.getter(new Name(function.name, library))); function.memberName? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1501: new FletchRegistry(compiler, compiler.globalDependencies); ? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1723: return transformed; Shouldn't return transformed if it wasn't modified, right? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_class_builder.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_class_builder.dart:159: var getter = new Selector.getter(new Name(field.name, field.library)); field.memberName? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_compiler_implementation.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:62: Extra line. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:263: void reportError( You need to move this to the reporter class, right? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_context.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_context.dart:212: new FletchRegistry(compiler, null); Fits one line? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_context.dart:257: Extra line. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_enqueuer.dart:212: compiler.globalDependencies); I don't understand this change. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_registry.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_registry.dart:138: final GlobalDependencyRegistry globalRegistry; I don't understand this change. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_system_builder.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_system_builder.dart:506: currentClass.forEachLocalMember(addIfField); Why not forEachInstanceField? https://codereview.chromium.org/1450393002/diff/40001/tests/corelib.status File tests/corelib.status (left): https://codereview.chromium.org/1450393002/diff/40001/tests/corelib.status#ol... tests/corelib.status:21: # Function.apply Why remove this? https://codereview.chromium.org/1450393002/diff/40001/tests/language.status File tests/language.status (right): https://codereview.chromium.org/1450393002/diff/40001/tests/language.status#n... tests/language.status:440: if_null_assignment_behavior_test/29: Crash Let's investigate this. Would like to avoid adding new crashes. https://codereview.chromium.org/1450393002/diff/40001/tests/language.status#n... tests/language.status:603: prefix_assignment_test/01: Crash Ditto.
THANK YOU! I don't have additional comments to Peters :) https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:1576: if (falseLabel != null) assembler.branch(falseLabel); Did this work before?
lgtm https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/clo... File pkg/fletchc/lib/src/closure_environment.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/clo... pkg/fletchc/lib/src/closure_environment.dart:751: void visitTypeAnnotation(TypeAnnotation node) { Add a comment about why this is needed. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:457: registerStaticUse(new StaticUse.foreignUse(element)); Add TODO for me to use a precise StaticUse in all cases in this file. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:1302: // TODO(ajohnsen): Remove hack - dart2js has a problem with generating Is this still needed? If so, please update the TODO with an example of when. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:1426: // selectors in initializer bodies. Update comment like mentioned above. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:2775: new Selector.call(Names.moveNext, CallStructure.NO_ARGS)); This is Selectors.moveNext
I still need to update sha-files for the binaries. https://codereview.chromium.org/1450393002/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/1450393002/diff/40001/DEPS#newcode37 DEPS:37: On 2015/11/17 16:44:08, ahe wrote: > Extra line. Done. https://codereview.chromium.org/1450393002/diff/40001/lib/core/core_patch.dart File lib/core/core_patch.dart (right): https://codereview.chromium.org/1450393002/diff/40001/lib/core/core_patch.dar... lib/core/core_patch.dart:674: @patch static String _uriEncode( On 2015/11/17 16:44:08, ahe wrote: > Where is this code from? Is it a copy of something? This is the version that was in lib/core/uri.dart before it was made external https://codereview.chromium.org/1450393002/diff/40001/lib/system/system.dart File lib/system/system.dart (right): https://codereview.chromium.org/1450393002/diff/40001/lib/system/system.dart#... lib/system/system.dart:89: class Patch { On 2015/11/17 16:44:08, ahe wrote: > What is this class for? I first wanted to make a patch class, decided not to halfway, and forgot to remove the class. Removed. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/increme... File pkg/fletchc/lib/incremental/library_updater.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/increme... pkg/fletchc/lib/incremental/library_updater.dart:796: if (before.isMalformed || On 2015/11/17 16:44:08, ahe wrote: > What does isMalformed mean? It's basically a rename of what was before isErroneous. It covers elements that are corrupted in some way, even though they might not be represented by an erroneous element https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/clo... File pkg/fletchc/lib/src/closure_environment.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/clo... pkg/fletchc/lib/src/closure_environment.dart:751: void visitTypeAnnotation(TypeAnnotation node) { On 2015/11/19 10:17:26, Johnni Winther wrote: > Add a comment about why this is needed. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:457: registerStaticUse(new StaticUse.foreignUse(element)); On 2015/11/19 10:17:26, Johnni Winther wrote: > Add TODO for me to use a precise StaticUse in all cases in this file. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:1302: // TODO(ajohnsen): Remove hack - dart2js has a problem with generating On 2015/11/19 10:17:26, Johnni Winther wrote: > Is this still needed? If so, please update the TODO with an example of when. Every test fails without this, so I removed it. anders, is that ok? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:1426: // selectors in initializer bodies. On 2015/11/19 10:17:26, Johnni Winther wrote: > Update comment like mentioned above. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:1576: if (falseLabel != null) assembler.branch(falseLabel); On 2015/11/18 06:46:55, Anders Johnsen wrote: > Did this work before? No - but no tests triggered it before. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:2775: new Selector.call(Names.moveNext, CallStructure.NO_ARGS)); On 2015/11/19 10:17:26, Johnni Winther wrote: > This is Selectors.moveNext Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:3180: new List(); On 2015/11/17 16:44:08, ahe wrote: > ? Oops. Removed https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/cod... pkg/fletchc/lib/src/codegen_visitor.dart:3223: void registerDynamicUse(DynamicUse use) { On 2015/11/17 16:44:08, ahe wrote: > This doesn't seem like the right API to me. We shouldn't be creating a bunch of > DynamicUse instances just to throw them away again. That's what will happen when > generating debug information. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... File pkg/fletchc/lib/src/debug_info_constructor_codegen.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... pkg/fletchc/lib/src/debug_info_constructor_codegen.dart:68: new FletchRegistry(compiler, null), On 2015/11/17 16:44:08, ahe wrote: > This is a problem. removed https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... File pkg/fletchc/lib/src/debug_registry.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/deb... pkg/fletchc/lib/src/debug_registry.dart:55: return null; On 2015/11/17 16:44:08, ahe wrote: > Throw instead? Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... File pkg/fletchc/lib/src/driver/developer.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... pkg/fletchc/lib/src/driver/developer.dart:421: return null; On 2015/11/17 16:44:09, ahe wrote: > Why this change? To shut up the analyzer. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... File pkg/fletchc/lib/src/driver/driver_main.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dri... pkg/fletchc/lib/src/driver/driver_main.dart:460: // executable (the C++ program). On 2015/11/17 16:44:09, ahe wrote: > Why this change? To shut up the analyzer. See: https://github.com/dart-lang/sdk/issues/24880. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dyn... File pkg/fletchc/lib/src/dynamic_call_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/dyn... pkg/fletchc/lib/src/dynamic_call_enqueuer.dart:199: void enqueueSelector(DynamicUse use) { On 2015/11/17 16:44:09, ahe wrote: > We need to change this. Not sure how yet. The method name is wrong. I guess that is for another CL. Is there a TODO you would like me to write? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/enq... File pkg/fletchc/lib/src/enqueuer_mixin.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/enq... pkg/fletchc/lib/src/enqueuer_mixin.dart:11: class EnqueuerMixin implements CodegenEnqueuer { On 2015/11/17 16:44:09, ahe wrote: > Not sure about this. Ok, lets discuss it. My idea was to get warnings if CodegenEnqueuer adds more methods that we want to implement https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/enq... pkg/fletchc/lib/src/enqueuer_mixin.dart:175: @override On 2015/11/17 16:44:09, ahe wrote: > Please don't use @override, or use it consistently. I tried to use it consistently in the code I added. This CL is already very big, and it is supposed to be the smallest sensible change that makes the roll work. That said I'm happy to add it to the rest of the methods. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_backend.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:60: import 'package:compiler/src/universe/use.dart' show DynamicUse, StaticUse, TypeUse, TypeUseKind; On 2015/11/17 16:44:09, ahe wrote: > Long line. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:127: import 'package:compiler/src/universe/world_impact.dart' show TransformedWorldImpact, WorldImpact, WorldImpactBuilder; On 2015/11/17 16:44:09, ahe wrote: > long line. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:276: Identifiers.call); On 2015/11/17 16:44:09, ahe wrote: > Fits on one line? Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:472: // void onMapLiteral(Registry registry, DartType type, bool isConstant) { On 2015/11/17 16:44:09, ahe wrote: > Remove? Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:725: new FletchRegistry(compiler, null), On 2015/11/17 16:44:09, ahe wrote: > I think this is wrong. I changed the constructor to not take a registry Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:739: new FletchRegistry(compiler, null), On 2015/11/17 16:44:09, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:750: new FletchRegistry(compiler, null), On 2015/11/17 16:44:09, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:911: compiler.reporter.internalError(element, "Couldn't find tear-off stub"); On 2015/11/17 16:44:09, ahe wrote: > Long line. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1190: new FletchRegistry(compiler, null); On 2015/11/17 16:44:09, ahe wrote: > Fits on one line? Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1320: new Selector.getter(new Name(function.name, library))); On 2015/11/17 16:44:09, ahe wrote: > function.memberName? There is no FunctionBase.memberName - and FunctionBase.element does not always exist. Added TODO https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1501: new FletchRegistry(compiler, compiler.globalDependencies); On 2015/11/17 16:44:09, ahe wrote: > ? Fixed https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1723: return transformed; On 2015/11/17 16:44:09, ahe wrote: > Shouldn't return transformed if it wasn't modified, right? This follows the pattern from dart2js. Johnni, any input? https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_class_builder.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_class_builder.dart:159: var getter = new Selector.getter(new Name(field.name, field.library)); On 2015/11/17 16:44:09, ahe wrote: > field.memberName? Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_compiler_implementation.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:62: On 2015/11/17 16:44:09, ahe wrote: > Extra line. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:263: void reportError( On 2015/11/17 16:44:09, ahe wrote: > You need to move this to the reporter class, right? Right. I made a wrapper around the compilers diagnosticreporter. A bit clunky - but the best I could come up with. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_context.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_context.dart:212: new FletchRegistry(compiler, null); On 2015/11/17 16:44:09, ahe wrote: > Fits one line? Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_context.dart:257: On 2015/11/17 16:44:09, ahe wrote: > Extra line. Done. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_enqueuer.dart:212: compiler.globalDependencies); On 2015/11/17 16:44:09, ahe wrote: > I don't understand this change. FletchRegistry constructor no longer takes a dependency registry. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_registry.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_registry.dart:138: final GlobalDependencyRegistry globalRegistry; On 2015/11/17 16:44:09, ahe wrote: > I don't understand this change. This is no longer needed. Fixed. https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_system_builder.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_system_builder.dart:506: currentClass.forEachLocalMember(addIfField); On 2015/11/17 16:44:09, ahe wrote: > Why not forEachInstanceField? forEachInstanceField has in the end calls forEachLocalMember via forEachMember using a instancefilter, and a conditional on superAndInjected. I thought this was simpler and more direct. But I'm happy to change it. https://codereview.chromium.org/1450393002/diff/40001/tests/corelib.status File tests/corelib.status (left): https://codereview.chromium.org/1450393002/diff/40001/tests/corelib.status#ol... tests/corelib.status:21: # Function.apply On 2015/11/17 16:44:10, ahe wrote: > Why remove this? No clue - must have happened by mistake. Reverted https://codereview.chromium.org/1450393002/diff/40001/tests/language.status File tests/language.status (right): https://codereview.chromium.org/1450393002/diff/40001/tests/language.status#n... tests/language.status:440: if_null_assignment_behavior_test/29: Crash On 2015/11/17 16:44:10, ahe wrote: > Let's investigate this. Would like to avoid adding new crashes. These are corresponding to crashes in dart2js. They have dissappeared now https://codereview.chromium.org/1450393002/diff/40001/tests/language.status#n... tests/language.status:603: prefix_assignment_test/01: Crash On 2015/11/17 16:44:10, ahe wrote: > Ditto. ditto
@ahe would you PTAL at the changes to FletchRegistry, and the updates to third_party/bin/README.md? @johnniwinther do you have comments regarding the ImpactTransformer? @ajohnsen Do the removed checks from the codegen_visitor look fine to you?
https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_backend.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1723: return transformed; On 2015/11/19 14:33:47, sigurdm wrote: > On 2015/11/17 16:44:09, ahe wrote: > > Shouldn't return transformed if it wasn't modified, right? > > This follows the pattern from dart2js. > > Johnni, any input? Just use [worldImpact] when not modified.
Yep, that part lgtm! https://codereview.chromium.org/1450393002/diff/100001/pkg/fletchc/lib/src/co... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/100001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:1364: new Selector.call(Names.call, callStructure)); One line?
https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... File pkg/fletchc/lib/src/fletch_backend.dart (right): https://codereview.chromium.org/1450393002/diff/40001/pkg/fletchc/lib/src/fle... pkg/fletchc/lib/src/fletch_backend.dart:1723: return transformed; On 2015/11/19 14:56:04, Johnni Winther wrote: > On 2015/11/19 14:33:47, sigurdm wrote: > > On 2015/11/17 16:44:09, ahe wrote: > > > Shouldn't return transformed if it wasn't modified, right? > > > > This follows the pattern from dart2js. > > > > Johnni, any input? > > Just use [worldImpact] when not modified. Done https://codereview.chromium.org/1450393002/diff/100001/pkg/fletchc/lib/src/co... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/100001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:1364: new Selector.call(Names.call, callStructure)); On 2015/11/19 15:09:22, Anders Johnsen wrote: > One line? Done.
Description was changed from ========== Roll sdk dependency to 8e49ae20ffc997a705bff3167394f3e47c982c1a Includes updates to fletchc to adapt to changes in the dart2js frontend. ========== to ========== Roll sdk dependency to 34357cdad108dcba734949bd13bd28c76ea285e0 Includes updates to fletchc to adapt to changes in the dart2js frontend. ==========
Now the binary sha's are also updated.
@ahe does this have green light from you?
Go for l(a)unch (aka LGTM)
Message was sent while issue was closed.
Committed patchset #8 (id:180001) manually as 9a7963cf02e632b68cc9e936fb5879b5da7864ec (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Roll sdk dependency to 34357cdad108dcba734949bd13bd28c76ea285e0 Includes updates to fletchc to adapt to changes in the dart2js frontend. R=ajohnsen@google.com, johnniwinther@google.com Committed: https://github.com/dart-lang/fletch/commit/9a7963cf02e632b68cc9e936fb5879b5da... ========== to ========== Roll sdk dependency to 34357cdad108dcba734949bd13bd28c76ea285e0 Includes updates to fletchc to adapt to changes in the dart2js frontend. R=ajohnsen@google.com, johnniwinther@google.com Committed: https://github.com/dart-lang/fletch/commit/9a7963cf02e632b68cc9e936fb5879b5da... Reverted: https://github.com/dart-lang/fletch/commit/4faa68c323d39de9c20f3aa58c76e8418a... ==========
Updated status files. PTAL (patch 9)
lgtm
Message was sent while issue was closed.
Committed patchset #9 (id:200001) manually as 3b5c04dca91c04f05c762e5eb98ca1dd93de7ce2 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1450393002/diff/200001/DEPS File DEPS (right): https://codereview.chromium.org/1450393002/diff/200001/DEPS#newcode26 DEPS:26: # 3. Upload new binaries and update the `third_party/bin` sha-hash-files as Also `tools/testing/bin/`. https://codereview.chromium.org/1450393002/diff/200001/lib/system/system.dart File lib/system/system.dart (right): https://codereview.chromium.org/1450393002/diff/200001/lib/system/system.dart... lib/system/system.dart:89: const patch = "patch"; I think this would naturally group with [native] on line 16. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/fletch... File pkg/fletchc/lib/fletch_compiler.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/fletch... pkg/fletchc/lib/fletch_compiler.dart:37: import 'package:compiler/src/apiimpl.dart' as apiimpl; I'd change this to: import 'package:compiler/src/apiimpl.dart' show CompilerImpl; https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... File pkg/fletchc/lib/src/closure_environment.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... pkg/fletchc/lib/src/closure_environment.dart:103: if (element.node != null) element.resolvedAst.node.accept(this); What is the difference between element.node and element.resolvedAst.node? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... pkg/fletchc/lib/src/closure_environment.dart:749: @override Please don't use @override. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... pkg/fletchc/lib/src/closure_environment.dart:751: // This is to avoid the inherited implementation that visits children and Indentation. Also, TODO. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:378: void registerStaticUse(StaticUse use); StaticUse (like DynamicUse) is problematic to use here for the same reason as DynamicUse: we construct these objects just to throw them away again immediately. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:403: registerDynamicUse(new Selector.getter(name)); This shouldn't create new objects. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:411: registerDynamicUse(new Selector.setter(name)); Ditto. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:455: registerStaticUse(new StaticUse.foreignUse(element)); Ditto. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:463: registerStaticUse(new StaticUse.foreignUse(constructor)); Ditto. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:1081: registerStaticUse(new StaticUse.foreignUse(function)); Ditto. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:1511: void doStaticFieldSet( Why this change? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/de... File pkg/fletchc/lib/src/debug_registry.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/de... pkg/fletchc/lib/src/debug_registry.dart:24: StaticUse; This file should not import these classes. That's one way to ensure that we don't create objects that we don't need. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/de... pkg/fletchc/lib/src/debug_registry.dart:59: throw null; What? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dr... File pkg/fletchc/lib/src/driver/developer.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dr... pkg/fletchc/lib/src/driver/developer.dart:460: return null; What is this for? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dy... File pkg/fletchc/lib/src/dynamic_call_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dy... pkg/fletchc/lib/src/dynamic_call_enqueuer.dart:14: DynamicUse; This class should not be used here. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_backend.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:472: registry.registerDynamicUse(new Selector.fromElement(element)); This isn't a dynamic use, and it is a problem for tree shaking that it is registered as such. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:934: registry.registerStaticUse(new StaticUse.foreignUse(fletchSystemEntry)); Should not create an instance of StaticUse. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1040: ..registerDynamicUse(new Selector.callClosure(0)) It's somewhat confusing that FletchRegistry uses the same method names as the ones that take DynamicUse. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1183: registry.registerStaticUse(new StaticUse.foreignUse(function)); Shouldn't instantiate StaticUse. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1314: new Selector.getter(name)); Also, we shouldn't allocate a selector. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1683: Extra line. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_class_builder.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:164: var setter = new Selector.setter(new Name(field.name, field.library)); Why not field.memberName? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:281: var getter = new Selector.getter(new Name(field.name, field.library)); Ditto https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:286: var setter = new Selector.setter(new Name(field.name, field.library)); Ditto https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:296: new Selector.getter(new Name(field.name, field.library)); Ditto https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:302: new Selector.setter(new Name(field.name, field.library)); Ditto https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_compiler_implementation.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:18: GlobalDependencyRegistry; Unused import. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:36: import 'package:compiler/src/diagnostics/source_span.dart' show Add newline before import. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:62: import 'package:compiler/src/diagnostics/diagnostic_listener.dart'; Newlines between imports. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:141: @override Please dont use @override. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:175: @override Ditto. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:246: void reportVerboseInfo( This should probably be moved to our reporter object? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:255: reportDiagnostic(new DiagnosticMessage(span, node, message), Why is reportDiagnostic not called on reporter? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:315: class FletchDiagnosticReporter extends DiagnosticReporter { Move to own file. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:315: class FletchDiagnosticReporter extends DiagnosticReporter { If you used implements, you wouldn't need all those noisy @override declarations. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:316: DiagnosticReporter _internalReporter; Please don't use library privacy when you're not implementing user-facing API. It only serves to: * Make life harder for your co-workers. * Prevent testing. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:321: DiagnosticMessage createMessage(Spannable spannable, Add newline after ( for consistency with code style used elsewhere in these files. Many times below. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_context.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_context.dart:75: Map<Name, String> nameToSymbol = <Name, String>{}; Unused? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_context.dart:139: String name, Are we using the right API on this method? Could it take a Name instead? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:72: import 'dart:developer'; What is this used for? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:73: import 'package:compiler/src/diagnostics/diagnostic_listener.dart'; Please use show, here and on all imports below. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:188: void registerStaticUse(StaticUse staticUse) { Since I don't think we should be instantiating objects to throw them away again, I don't think this is the right API. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:244: void registerDynamicUse(DynamicUse use) { Ditto. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_registry.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_registry.dart:21: StaticUse; Shouldn't be imported here. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_registry.dart:82: void registerStaticUse(StaticUse staticUse) { Also think this is the wrong API. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_registry.dart:92: world.registerDynamicUse(new DynamicUse(selector, null)); Shouldn't instantiate DynamicUse. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_system_builder.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_system_builder.dart:491: void addIfField(MemberElement member) { Rename to addIfInstanceField.
Message was sent while issue was closed.
https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... File pkg/fletchc/lib/src/closure_environment.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... pkg/fletchc/lib/src/closure_environment.dart:103: if (element.node != null) element.resolvedAst.node.accept(this); On 2015/12/01 10:12:12, ahe wrote: > What is the difference between element.node and element.resolvedAst.node? If [element] is patched `element.node` is the external declaration. `element.resolvedAst.node` is always the one that corresponds to the TreeElements.
Message was sent while issue was closed.
Patchset #10 (id:220001) has been deleted
Message was sent while issue was closed.
Thanks for the review. I did a follow-up here: https://codereview.chromium.org/1491823004. Also I uploaded the followup as a patch in this CL, so it can be seen in context. https://codereview.chromium.org/1450393002/diff/200001/DEPS File DEPS (right): https://codereview.chromium.org/1450393002/diff/200001/DEPS#newcode26 DEPS:26: # 3. Upload new binaries and update the `third_party/bin` sha-hash-files as On 2015/12/01 10:12:11, ahe wrote: > Also `tools/testing/bin/`. Done. https://codereview.chromium.org/1450393002/diff/200001/lib/system/system.dart File lib/system/system.dart (right): https://codereview.chromium.org/1450393002/diff/200001/lib/system/system.dart... lib/system/system.dart:89: const patch = "patch"; On 2015/12/01 10:12:12, ahe wrote: > I think this would naturally group with [native] on line 16. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/fletch... File pkg/fletchc/lib/fletch_compiler.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/fletch... pkg/fletchc/lib/fletch_compiler.dart:37: import 'package:compiler/src/apiimpl.dart' as apiimpl; On 2015/12/01 10:12:12, ahe wrote: > I'd change this to: > > import 'package:compiler/src/apiimpl.dart' show > CompilerImpl; Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... File pkg/fletchc/lib/src/closure_environment.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... pkg/fletchc/lib/src/closure_environment.dart:749: @override On 2015/12/01 10:12:12, ahe wrote: > Please don't use @override. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/cl... pkg/fletchc/lib/src/closure_environment.dart:751: // This is to avoid the inherited implementation that visits children and On 2015/12/01 10:12:12, ahe wrote: > Indentation. Also, TODO. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... File pkg/fletchc/lib/src/codegen_visitor.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:378: void registerStaticUse(StaticUse use); On 2015/12/01 10:12:12, ahe wrote: > StaticUse (like DynamicUse) is problematic to use here for the same reason as > DynamicUse: we construct these objects just to throw them away again > immediately. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:403: registerDynamicUse(new Selector.getter(name)); On 2015/12/01 10:12:12, ahe wrote: > This shouldn't create new objects. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:411: registerDynamicUse(new Selector.setter(name)); On 2015/12/01 10:12:12, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:455: registerStaticUse(new StaticUse.foreignUse(element)); On 2015/12/01 10:12:12, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:463: registerStaticUse(new StaticUse.foreignUse(constructor)); On 2015/12/01 10:12:12, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:1081: registerStaticUse(new StaticUse.foreignUse(function)); On 2015/12/01 10:12:12, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/co... pkg/fletchc/lib/src/codegen_visitor.dart:1511: void doStaticFieldSet( On 2015/12/01 10:12:12, ahe wrote: > Why this change? Not sure - reverted. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/de... File pkg/fletchc/lib/src/debug_registry.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/de... pkg/fletchc/lib/src/debug_registry.dart:24: StaticUse; On 2015/12/01 10:12:12, ahe wrote: > This file should not import these classes. That's one way to ensure that we > don't create objects that we don't need. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/de... pkg/fletchc/lib/src/debug_registry.dart:59: throw null; On 2015/12/01 10:12:12, ahe wrote: > What? To shut up the analyzer. Changed https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dr... File pkg/fletchc/lib/src/driver/developer.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dr... pkg/fletchc/lib/src/driver/developer.dart:460: return null; On 2015/12/01 10:12:12, ahe wrote: > What is this for? To shut up the analyzer. If there are no more commands, the right thing seems to be to explicitly return null as there is no address. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dy... File pkg/fletchc/lib/src/dynamic_call_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/dy... pkg/fletchc/lib/src/dynamic_call_enqueuer.dart:14: DynamicUse; On 2015/12/01 10:12:12, ahe wrote: > This class should not be used here. DynamicUse is the new name of UniverseSelector. This is not a new thing I introduced. I can add a TODO if you want? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_backend.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:472: registry.registerDynamicUse(new Selector.fromElement(element)); On 2015/12/01 10:12:12, ahe wrote: > This isn't a dynamic use, and it is a problem for tree shaking that it is > registered as such. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:934: registry.registerStaticUse(new StaticUse.foreignUse(fletchSystemEntry)); On 2015/12/01 10:12:12, ahe wrote: > Should not create an instance of StaticUse. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1040: ..registerDynamicUse(new Selector.callClosure(0)) On 2015/12/01 10:12:12, ahe wrote: > It's somewhat confusing that FletchRegistry uses the same method names as the > ones that take DynamicUse. I renamed it to registerDynamicSelector https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1183: registry.registerStaticUse(new StaticUse.foreignUse(function)); On 2015/12/01 10:12:12, ahe wrote: > Shouldn't instantiate StaticUse. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1314: new Selector.getter(name)); On 2015/12/01 10:12:12, ahe wrote: > Also, we shouldn't allocate a selector. Added to TODO https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_backend.dart:1683: On 2015/12/01 10:12:12, ahe wrote: > Extra line. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_class_builder.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:164: var setter = new Selector.setter(new Name(field.name, field.library)); On 2015/12/01 10:12:12, ahe wrote: > Why not field.memberName? Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:281: var getter = new Selector.getter(new Name(field.name, field.library)); On 2015/12/01 10:12:12, ahe wrote: > Ditto Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:286: var setter = new Selector.setter(new Name(field.name, field.library)); On 2015/12/01 10:12:13, ahe wrote: > Ditto Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:296: new Selector.getter(new Name(field.name, field.library)); On 2015/12/01 10:12:12, ahe wrote: > Ditto Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_class_builder.dart:302: new Selector.setter(new Name(field.name, field.library)); On 2015/12/01 10:12:12, ahe wrote: > Ditto Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_compiler_implementation.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:18: GlobalDependencyRegistry; On 2015/12/01 10:12:13, ahe wrote: > Unused import. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:36: import 'package:compiler/src/diagnostics/source_span.dart' show On 2015/12/01 10:12:13, ahe wrote: > Add newline before import. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:62: import 'package:compiler/src/diagnostics/diagnostic_listener.dart'; On 2015/12/01 10:12:13, ahe wrote: > Newlines between imports. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:141: @override On 2015/12/01 10:12:13, ahe wrote: > Please dont use @override. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:175: @override On 2015/12/01 10:12:13, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:246: void reportVerboseInfo( On 2015/12/01 10:12:13, ahe wrote: > This should probably be moved to our reporter object? I'll wait for Johnni to move it in the dart2js frontend https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:255: reportDiagnostic(new DiagnosticMessage(span, node, message), On 2015/12/01 10:12:13, ahe wrote: > Why is reportDiagnostic not called on reporter? There is not reportDiagnostic on reporter, but on compiler. Maybe johnni knows why? https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:315: class FletchDiagnosticReporter extends DiagnosticReporter { On 2015/12/01 10:12:13, ahe wrote: > Move to own file. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:315: class FletchDiagnosticReporter extends DiagnosticReporter { On 2015/12/01 10:12:13, ahe wrote: > If you used implements, you wouldn't need all those noisy @override > declarations. I don't understand the argument. I use extends to inherit functionality from DiagnosticReporter. I don't see how that has anything to do with @override. I disagree that @override annotations are noisy, and think they serve a purpose on a similar level to type annotations. But I will remove them as I see this is not a shared belief. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:316: DiagnosticReporter _internalReporter; On 2015/12/01 10:12:13, ahe wrote: > Please don't use library privacy when you're not implementing user-facing API. > > It only serves to: > > * Make life harder for your co-workers. > > * Prevent testing. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_compiler_implementation.dart:321: DiagnosticMessage createMessage(Spannable spannable, On 2015/12/01 10:12:13, ahe wrote: > Add newline after ( for consistency with code style used elsewhere in these > files. Many times below. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_context.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_context.dart:75: Map<Name, String> nameToSymbol = <Name, String>{}; On 2015/12/01 10:12:13, ahe wrote: > Unused? Yes - removed https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_context.dart:139: String name, On 2015/12/01 10:12:13, ahe wrote: > Are we using the right API on this method? Could it take a Name instead? Done https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_enqueuer.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:72: import 'dart:developer'; On 2015/12/01 10:12:13, ahe wrote: > What is this used for? That was stary debugging code - removed https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:73: import 'package:compiler/src/diagnostics/diagnostic_listener.dart'; On 2015/12/01 10:12:13, ahe wrote: > Please use show, here and on all imports below. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:188: void registerStaticUse(StaticUse staticUse) { On 2015/12/01 10:12:13, ahe wrote: > Since I don't think we should be instantiating objects to throw them away again, > I don't think this is the right API. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_enqueuer.dart:244: void registerDynamicUse(DynamicUse use) { On 2015/12/01 10:12:13, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_registry.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_registry.dart:21: StaticUse; On 2015/12/01 10:12:13, ahe wrote: > Shouldn't be imported here. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_registry.dart:82: void registerStaticUse(StaticUse staticUse) { On 2015/12/01 10:12:13, ahe wrote: > Also think this is the wrong API. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_registry.dart:92: world.registerDynamicUse(new DynamicUse(selector, null)); On 2015/12/01 10:12:13, ahe wrote: > Shouldn't instantiate DynamicUse. Done. https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... File pkg/fletchc/lib/src/fletch_system_builder.dart (right): https://codereview.chromium.org/1450393002/diff/200001/pkg/fletchc/lib/src/fl... pkg/fletchc/lib/src/fletch_system_builder.dart:491: void addIfField(MemberElement member) { On 2015/12/01 10:12:13, ahe wrote: > Rename to addIfInstanceField. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
