Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1099703003:
Treat our type objects as core.Type (Closed)
|
Created:
5 years, 8 months ago by vsm Modified:
5 years, 7 months ago CC:
dev-compiler+reviews_dartlang.org Base URL:
https://github.com/dart-lang/dev_compiler.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionTreat our type objects as core.Type and other minor fixes
R=leafp@google.com
Committed: https://github.com/dart-lang/dev_compiler/commit/db2f8185aaf7d3c816e71e3cb052e365b0788069
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updated CL #Messages
Total messages: 9 (1 generated)
vsm@google.com changed reviewers: + jmesserly@google.com, leafp@google.com
https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... File lib/src/codegen/js_codegen.dart (right): https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1050: dynamic memberName = name; Note: this is to map exports._foo as exports[_foo], etc.
https://codereview.chromium.org/1099703003/diff/1/lib/runtime/dart_runtime.js File lib/runtime/dart_runtime.js (right): https://codereview.chromium.org/1099703003/diff/1/lib/runtime/dart_runtime.js... lib/runtime/dart_runtime.js:212: result = t1.prototype instanceof core.Type || any reason not to include this in isSubtype_ ? I was trying to keep that one as the "real" method, with this one just being the memoize glue https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... File lib/src/codegen/js_codegen.dart (right): https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1050: dynamic memberName = name; On 2015/04/24 00:12:11, vsm wrote: > Note: this is to map exports._foo as exports[_foo], etc. Interesting, how is that used? Would anybody have the key to access it? My assumption so far was only member types would use symbols, it doesn't seem as useful at the library level. I didn't see any generated code changes, is there a sample of what this looks like?
https://codereview.chromium.org/1099703003/diff/1/lib/runtime/dart_runtime.js File lib/runtime/dart_runtime.js (right): https://codereview.chromium.org/1099703003/diff/1/lib/runtime/dart_runtime.js... lib/runtime/dart_runtime.js:212: result = t1.prototype instanceof core.Type || On 2015/04/24 00:43:14, John Messerly wrote: > any reason not to include this in isSubtype_ ? I was trying to keep that one as > the "real" method, with this one just being the memoize glue Yes - the helper is recursive with t2 staying constant, so we'd be repeating this check every time. https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... File lib/src/codegen/js_codegen.dart (right): https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1050: dynamic memberName = name; On 2015/04/24 00:43:14, John Messerly wrote: > On 2015/04/24 00:12:11, vsm wrote: > > Note: this is to map exports._foo as exports[_foo], etc. > > Interesting, how is that used? Would anybody have the key to access it? My > assumption so far was only member types would use symbols, it doesn't seem as > useful at the library level. > > I didn't see any generated code changes, is there a sample of what this looks > like? Yes, I was hitting this in angular (application.js) - no cases in the sdk. Here's the original generated code: Def site: let _rootBindings = Symbol('_rootBindings'); dart.defineLazyProperties(exports, { get [_rootBindings]() { return [binding.bind(reflector.Reflector).toValue(reflection.reflector)]; }, Use site: exports._rootInjector = new injector.Injector(exports._rootBindings); The change produces exports[_rootBindings] at the use site instead.
This lgtm for now if we want to use this to make progress. Per our conversation, I think we may want to make a more uniform type representation as a starting place, see where that leaves us, and then move towards eliminating the explicit representations where js gives us enough hooks to construct it implicitly.
https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... File lib/src/codegen/js_codegen.dart (right): https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1050: dynamic memberName = name; On 2015/04/24 13:25:44, vsm wrote: > On 2015/04/24 00:43:14, John Messerly wrote: > > On 2015/04/24 00:12:11, vsm wrote: > > > Note: this is to map exports._foo as exports[_foo], etc. > > > > Interesting, how is that used? Would anybody have the key to access it? My > > assumption so far was only member types would use symbols, it doesn't seem as > > useful at the library level. > > > > I didn't see any generated code changes, is there a sample of what this looks > > like? > > Yes, I was hitting this in angular (application.js) - no cases in the sdk. > Here's the original generated code: > > Def site: > let _rootBindings = Symbol('_rootBindings'); > dart.defineLazyProperties(exports, { > get [_rootBindings]() { > return [binding.bind(reflector.Reflector).toValue(reflection.reflector)]; > }, > > Use site: > exports._rootInjector = new injector.Injector(exports._rootBindings); > > The change produces exports[_rootBindings] at the use site instead. Aha, lazy top-level field. Interesting. I suppose we could change how it's generated (to _rootBindings), but then again, I suppose this pattern does give a bit more protection for private library fields. And maybe it is consistent if all private state is like that? Anyway a few small nits: * can we use `JS.Expression` instead of `dynamic`? * add a comment explaining how this happens? https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1051: if (name.startsWith('_')) { in the if body: assert(element.library == currentLibrary); https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1052: memberName = _privateNames.putIfAbsent( Maybe should go through emitMemberName then, with isStatic: true? I think we ended up here because _emitLazyFields calls emitMemberName.
https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... File lib/src/codegen/js_codegen.dart (right): https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1051: if (name.startsWith('_')) { On 2015/04/24 21:23:23, John Messerly wrote: > in the if body: > > assert(element.library == currentLibrary); Maybe won't need with emitMemberName suggestion though. In that case it would be: assert(element.isPublic || element.library == currentLibrary); var memberName = emitMemberName(...) return js.call(...);
https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... File lib/src/codegen/js_codegen.dart (right): https://codereview.chromium.org/1099703003/diff/1/lib/src/codegen/js_codegen.... lib/src/codegen/js_codegen.dart:1050: dynamic memberName = name; On 2015/04/24 21:23:23, John Messerly wrote: > On 2015/04/24 13:25:44, vsm wrote: > > On 2015/04/24 00:43:14, John Messerly wrote: > > > On 2015/04/24 00:12:11, vsm wrote: > > > > Note: this is to map exports._foo as exports[_foo], etc. > > > > > > Interesting, how is that used? Would anybody have the key to access it? My > > > assumption so far was only member types would use symbols, it doesn't seem > as > > > useful at the library level. > > > > > > I didn't see any generated code changes, is there a sample of what this > looks > > > like? > > > > Yes, I was hitting this in angular (application.js) - no cases in the sdk. > > Here's the original generated code: > > > > Def site: > > let _rootBindings = Symbol('_rootBindings'); > > dart.defineLazyProperties(exports, { > > get [_rootBindings]() { > > return > [binding.bind(reflector.Reflector).toValue(reflection.reflector)]; > > }, > > > > Use site: > > exports._rootInjector = new injector.Injector(exports._rootBindings); > > > > The change produces exports[_rootBindings] at the use site instead. > > Aha, lazy top-level field. Interesting. I suppose we could change how it's > generated (to _rootBindings), but then again, I suppose this pattern does give a > bit more protection for private library fields. And maybe it is consistent if > all private state is like that? > > Anyway a few small nits: > * can we use `JS.Expression` instead of `dynamic`? > * add a comment explaining how this happens? Not sure I still need this with your recent change to emitMemberName. I'm reverting this change from this CL. I'll revisit - if needed - if I see it come up again.
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as db2f8185aaf7d3c816e71e3cb052e365b0788069 (presubmit successful). |
