Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824123003:
Make backend.setAnnotations (mostly) handle LibraryEntities. (Closed)
|
Created:
3 years, 8 months ago by Emily Fortuna Modified:
3 years, 8 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMake backend.setAnnotations (mostly) handle LibraryEntities.
BUG=
R=johnniwinther@google.com
Committed: https://github.com/dart-lang/sdk/commit/6c25a58dabfcb4c99d965952684193face1fd797
Patch Set 1 : . #
Total comments: 18
Patch Set 2 : comments #
Messages
Total messages: 10 (4 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
efortuna@google.com changed reviewers: + johnniwinther@google.com, sigmund@google.com
Implemented a handful of the things discussed here: https://docs.google.com/a/google.com/document/d/1KgBODP1rvvvJzbvnUMjaX56kv-DF... and then see comments where I wanted to make sure I'm going in the direction you're envisioning for Kernel/Node lookup. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... File pkg/compiler/lib/src/kernel/world_builder.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... pkg/compiler/lib/src/kernel/world_builder.dart:529: class KernelLookup { I'm guessing we want to have some sort of super interface where we can do the same thing for the AST nodes (instead of looking up on the element->nodes themselves)? I wanted to check before implementing too much though. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... File pkg/compiler/lib/src/native/resolver.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:47: class _KernelAnnotationProcessor implements AnnotationProcessor { this class would use the KernelLookup class, but I didn't implement any further until I got the go-ahead that some sort of lookup thing is what Johnni was envisioning.
lgtm https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... File pkg/compiler/lib/src/kernel/world_builder.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... pkg/compiler/lib/src/kernel/world_builder.dart:529: class KernelLookup { On 2017/04/19 00:59:55, Emily Fortuna wrote: > I'm guessing we want to have some sort of super interface where we can do the > same thing for the AST nodes (instead of looking up on the element->nodes > themselves)? I wanted to check before implementing too much though. This functionality is already in KernelWorldBuilder (through libraryIndex, classIndex, etc). Just leave out this class, I'll merge the _KernelAnnotationProcessor with the code I already have. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... File pkg/compiler/lib/src/native/resolver.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:38: : new _ElementAnnotationProcessor(compiler); Eventually we should use a strategy pattern for all these: abstract class FrontendStrategy { factory FrontendStrategy(Compiler compiler) => compiler.options.loadFromDill ? new _KernelStrategy() : new _ResolverStrategy(compiler); LibraryLoaderTask createLibraryLoaderTask(...); ElementEnvironment createElementEnvironment(); AnnotationProcessor createAnnotationProcessor(); ... } class ResolverStrategy implements FrontEndStrategy { final Compiler _compiler; ResolverStrategy(this._compiler); LibraryLoader createLibraryLoader(...) => new _LibraryLoaderTask(...); ElementEnvironment createElementEnvironment() => new _CompilerElementEnvironment(_compiler); AnnotationProcessor createAnnotationProcessor() => new _ElementAnnotationProcessor(_compiler); ... } class KernelStrategy implements FrontStrategy { KernelWorldBuilder _worldBuilder; KernelStrategy(); LibraryLoader createLibraryLoader(...) => new _DillLibraryLoaderTask(_worldBuilder, ...); ElementEnvironment createElementEnvironment() => _worldBuilder.elementEnvironment; AnnotationProcessor createAnnotationProcessor() => new _KernelAnnotationProcessor(_worldBuilder); ... } https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:71: LibraryEntity library, NativeBasicDataBuilder nativeBasicDataBuilder) { Just use LibraryElement here. This is a model dependent class anyway. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:81: LibraryEntity library, NativeBasicDataBuilder nativeBasicDataBuilder) { Ditto
Thanks Emily! https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... File pkg/compiler/lib/src/compiler.dart (left): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... pkg/compiler/lib/src/compiler.dart:599: .log('Enqueuing ${(library as LibraryElement).canonicalUri}'); thx! https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... File pkg/compiler/lib/src/compiler.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... pkg/compiler/lib/src/compiler.dart:387: if (loadedLibraries.rootLibrary is LibraryElement) { nit: I prefer to make this check as if (!options.useKernel) { ... since that's the root cause for why we would skip this. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... pkg/compiler/lib/src/compiler.dart:656: if (options.analyzeOnly) { nit - we can now remove the braces and make this a one-liner, too! https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... File pkg/compiler/lib/src/kernel/world_builder.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... pkg/compiler/lib/src/kernel/world_builder.dart:529: class KernelLookup { On 2017/04/19 08:39:51, Johnni Winther wrote: > On 2017/04/19 00:59:55, Emily Fortuna wrote: > > I'm guessing we want to have some sort of super interface where we can do the > > same thing for the AST nodes (instead of looking up on the element->nodes > > themselves)? I wanted to check before implementing too much though. > > This functionality is already in KernelWorldBuilder (through libraryIndex, > classIndex, etc). Just leave out this class, I'll merge the > _KernelAnnotationProcessor with the code I already have. Ah! some questions for Johnni... 1. is the idea to mimic what you have in lookupLibraryMember? (i.e. you would add a similar method like lookupLibraryKernelMember and directly return the ir node?) 2. I'd like to split KernelWorldBuilder or rename it. I feel like we should have a simple mechanism to map elements and nodes that is independent of a world builder - I imagine we'll create a world builder later in the compiler pipeline, and we can make some pieces of the compiler (like the annotation processor) independent of the world. Do you agree? https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... File pkg/compiler/lib/src/native/resolver.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:38: : new _ElementAnnotationProcessor(compiler); On 2017/04/19 08:39:51, Johnni Winther wrote: > Eventually we should use a strategy pattern for all these: > > abstract class FrontendStrategy { > factory FrontendStrategy(Compiler compiler) => > compiler.options.loadFromDill > ? new _KernelStrategy() > : new _ResolverStrategy(compiler); > > LibraryLoaderTask createLibraryLoaderTask(...); > ElementEnvironment createElementEnvironment(); > AnnotationProcessor createAnnotationProcessor(); > ... > } > > class ResolverStrategy implements FrontEndStrategy { > final Compiler _compiler; > > ResolverStrategy(this._compiler); > > LibraryLoader createLibraryLoader(...) => > new _LibraryLoaderTask(...); > ElementEnvironment createElementEnvironment() => > new _CompilerElementEnvironment(_compiler); > AnnotationProcessor createAnnotationProcessor() => > new _ElementAnnotationProcessor(_compiler); > ... > } > > class KernelStrategy implements FrontStrategy { > KernelWorldBuilder _worldBuilder; > > KernelStrategy(); > > LibraryLoader createLibraryLoader(...) => > new _DillLibraryLoaderTask(_worldBuilder, ...); > ElementEnvironment createElementEnvironment() => > _worldBuilder.elementEnvironment; > AnnotationProcessor createAnnotationProcessor() => > new _KernelAnnotationProcessor(_worldBuilder); > ... > } An alternative naming convention could be to rename Strategy as Module, Factory, or Injector (I'm borrowing names that are commonly used to refer to this in dependency-injection frameworks).
https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... File pkg/compiler/lib/src/compiler.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... pkg/compiler/lib/src/compiler.dart:387: if (loadedLibraries.rootLibrary is LibraryElement) { On 2017/04/19 15:36:29, Siggi Cherem (dart-lang) wrote: > nit: I prefer to make this check as > > if (!options.useKernel) { ... > > since that's the root cause for why we would skip this. Done. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/co... pkg/compiler/lib/src/compiler.dart:656: if (options.analyzeOnly) { On 2017/04/19 15:36:29, Siggi Cherem (dart-lang) wrote: > nit - we can now remove the braces and make this a one-liner, too! Done. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... File pkg/compiler/lib/src/kernel/world_builder.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... pkg/compiler/lib/src/kernel/world_builder.dart:529: class KernelLookup { On 2017/04/19 08:39:51, Johnni Winther wrote: > On 2017/04/19 00:59:55, Emily Fortuna wrote: > > I'm guessing we want to have some sort of super interface where we can do the > > same thing for the AST nodes (instead of looking up on the element->nodes > > themselves)? I wanted to check before implementing too much though. > > This functionality is already in KernelWorldBuilder (through libraryIndex, > classIndex, etc). Just leave out this class, I'll merge the > _KernelAnnotationProcessor with the code I already have. Okay. You're still not exposing the ir.Library and ir.Classes, though. I added methods to do that. Please let me know if you had envisioned something different. Please also fill us in given Siggi's questions. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... File pkg/compiler/lib/src/native/resolver.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:38: : new _ElementAnnotationProcessor(compiler); On 2017/04/19 15:36:29, Siggi Cherem (dart-lang) wrote: > On 2017/04/19 08:39:51, Johnni Winther wrote: > > Eventually we should use a strategy pattern for all these: > > > > abstract class FrontendStrategy { > > factory FrontendStrategy(Compiler compiler) => > > compiler.options.loadFromDill > > ? new _KernelStrategy() > > : new _ResolverStrategy(compiler); > > > > LibraryLoaderTask createLibraryLoaderTask(...); > > ElementEnvironment createElementEnvironment(); > > AnnotationProcessor createAnnotationProcessor(); > > ... > > } > > > > class ResolverStrategy implements FrontEndStrategy { > > final Compiler _compiler; > > > > ResolverStrategy(this._compiler); > > > > LibraryLoader createLibraryLoader(...) => > > new _LibraryLoaderTask(...); > > ElementEnvironment createElementEnvironment() => > > new _CompilerElementEnvironment(_compiler); > > AnnotationProcessor createAnnotationProcessor() => > > new _ElementAnnotationProcessor(_compiler); > > ... > > } > > > > class KernelStrategy implements FrontStrategy { > > KernelWorldBuilder _worldBuilder; > > > > KernelStrategy(); > > > > LibraryLoader createLibraryLoader(...) => > > new _DillLibraryLoaderTask(_worldBuilder, ...); > > ElementEnvironment createElementEnvironment() => > > _worldBuilder.elementEnvironment; > > AnnotationProcessor createAnnotationProcessor() => > > new _KernelAnnotationProcessor(_worldBuilder); > > ... > > } > > An alternative naming convention could be to rename Strategy as Module, > Factory, or Injector (I'm borrowing names that are commonly used to refer to > this in dependency-injection frameworks). I started doing this, but ultimately I removed it because it felt like I was grouping things together that were technically unrelated to each other together just because they were "the kernel way of doing things" which resulted in passing a "strategy object" around all the time (to the backend and elsewhere) instead of just what was needed. It felt a bit like a re-inventing of the compiler object. WDYT? https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:71: LibraryEntity library, NativeBasicDataBuilder nativeBasicDataBuilder) { On 2017/04/19 08:39:51, Johnni Winther wrote: > Just use LibraryElement here. This is a model dependent class anyway. Done. https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/na... pkg/compiler/lib/src/native/resolver.dart:81: LibraryEntity library, NativeBasicDataBuilder nativeBasicDataBuilder) { On 2017/04/19 08:39:51, Johnni Winther wrote: > Ditto Done.
Description was changed from ========== Make backend.setAnnotations (mostly) handle LibraryEntities. BUG= ========== to ========== Make backend.setAnnotations (mostly) handle LibraryEntities. BUG= R=johnniwinther@google.com Committed: https://github.com/dart-lang/sdk/commit/6c25a58dabfcb4c99d965952684193face1fd797 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) manually as 6c25a58dabfcb4c99d965952684193face1fd797 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... File pkg/compiler/lib/src/kernel/world_builder.dart (right): https://codereview.chromium.org/2824123003/diff/40001/pkg/compiler/lib/src/ke... pkg/compiler/lib/src/kernel/world_builder.dart:529: class KernelLookup { On 2017/04/19 15:36:29, Siggi Cherem (dart-lang) wrote: > On 2017/04/19 08:39:51, Johnni Winther wrote: > > On 2017/04/19 00:59:55, Emily Fortuna wrote: > > > I'm guessing we want to have some sort of super interface where we can do > the > > > same thing for the AST nodes (instead of looking up on the element->nodes > > > themselves)? I wanted to check before implementing too much though. > > > > This functionality is already in KernelWorldBuilder (through libraryIndex, > > classIndex, etc). Just leave out this class, I'll merge the > > _KernelAnnotationProcessor with the code I already have. > > Ah! some questions for Johnni... > > 1. is the idea to mimic what you have in lookupLibraryMember? (i.e. you would > add a similar method like lookupLibraryKernelMember and directly return the ir > node?) > > 2. I'd like to split KernelWorldBuilder or rename it. I feel like we should have > a simple mechanism to map elements and nodes that is independent of a world > builder - I imagine we'll create a world builder later in the compiler pipeline, > and we can make some pieces of the compiler (like the annotation processor) > independent of the world. Do you agree? 1. I want IR based reasoning to be confined to the kernel/world_builder library. History shows that if we expose the internal structures we will use them everywhere (think of how we use AST nodes). So _no_ public methods returning IR nodes, if avoidable. 2. You rarely need IR nodes without their relation to the element model, that is what KernelWorldBuilder is for. (And I will soon rename it to KernelElementBuilder to reflect that more clearly). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
