Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10690110:
dart2dart support for instantiatin simple top-level class: (Closed)
|
Created:
8 years, 5 months ago by Roman Modified:
8 years, 5 months ago Reviewers:
Anton Muhin CC:
reviews_dartlang.org Visibility:
Public. |
Descriptiondart2dart support for instantiatin simple top-level class:
class A {
}
main() {
new A();
}
Committed: https://code.google.com/p/dart/source/detail?r=9506
Patch Set 1 #
Total comments: 24
Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 7 (0 generated)
neat https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:17: Map<ClassElement, List<Element>> resolvedClasses; resolvedClasses doesn't sound correct. That's rather classMembers, wdyt? https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:17: Map<ClassElement, List<Element>> resolvedClasses; should it be List<Element> or Set<Element>? https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:24: resolvedClasses = new HashMap<ClassElement, List<Element>>(); no need to make default implementation explicit and it should go into intializer list. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:42: if (element.enclosingElement !== classElement) { assert? https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:47: if (resolvedElementsInClass == null) { nit: there is putIfAbsent thing which may encode this pattern nicely. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:59: if (element.isTopLevel()) { it rather should be an assert (see below regarding isMember()) https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:63: var enclosingClass = element.enclosingElement; it should probably be if (element.isMember()) https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:68: if (!enclosingClass.isTopLevel()) { I'd rather make it assert as classes must be top-level to my best knowledge. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:114: resolveClass(element); I would rather have something like: if (element.isMember()) { processMember(element); return; } https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:141: outputClass(classElement, resolvedElements, sb); if you made outputClass a closure, you can have nice: resolvedClasses.forEach(outputClass); or just inline it instead of adding a new method. https://chromiumcodereview.appspot.com/10690110/diff/1/tests/compiler/dart2js... File tests/compiler/dart2js/unparser_test.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/unparser_test.dart:132: testDart2Dart(src, (String s) => Expect.equals(src, s)); it might be time to factor out this repeating pattern.
https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:17: Map<ClassElement, List<Element>> resolvedClasses; On 2012/07/10 09:50:12, antonmuhin wrote: > resolvedClasses doesn't sound correct. That's rather classMembers, wdyt? renamed to resolvedClassMembers https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:17: Map<ClassElement, List<Element>> resolvedClasses; On 2012/07/10 09:50:12, antonmuhin wrote: > should it be List<Element> or Set<Element>? Correct! Changed to set https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:24: resolvedClasses = new HashMap<ClassElement, List<Element>>(); On 2012/07/10 09:50:12, antonmuhin wrote: > no need to make default implementation explicit and it should go into intializer > list. Done. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:42: if (element.enclosingElement !== classElement) { On 2012/07/10 09:50:12, antonmuhin wrote: > assert? Done. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:47: if (resolvedElementsInClass == null) { On 2012/07/10 09:50:12, antonmuhin wrote: > nit: there is putIfAbsent thing which may encode this pattern nicely. Awesome thing, thanks! https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:59: if (element.isTopLevel()) { On 2012/07/10 09:50:12, antonmuhin wrote: > it rather should be an assert (see below regarding isMember()) Done. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:63: var enclosingClass = element.enclosingElement; On 2012/07/10 09:50:12, antonmuhin wrote: > it should probably be if (element.isMember()) what do you mean by 'it'? This is a check whether enclosing element is a class. It may be another element kind in theory (another method maybe?), we can't process this case yet. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:68: if (!enclosingClass.isTopLevel()) { On 2012/07/10 09:50:12, antonmuhin wrote: > I'd rather make it assert as classes must be top-level to my best knowledge. Done. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:114: resolveClass(element); On 2012/07/10 09:50:12, antonmuhin wrote: > I would rather have something like: > > if (element.isMember()) { > processMember(element); > return; > } Done. https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:141: outputClass(classElement, resolvedElements, sb); On 2012/07/10 09:50:12, antonmuhin wrote: > if you made outputClass a closure, you can have nice: > resolvedClasses.forEach(outputClass); or just inline it instead of adding a new > method. I would prefer to leave it this way, if you don't mind. It looks more clear to reader what's happening than closure.
https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:17: Map<ClassElement, List<Element>> resolvedClasses; I'd rather not see word resolved here. It's used in resolvedElements because it's just a shortcut to resolution enqueuer. In your case, you're not resolving stuff any more. But up to you. On 2012/07/10 10:45:15, Roman wrote: > On 2012/07/10 09:50:12, antonmuhin wrote: > > resolvedClasses doesn't sound correct. That's rather classMembers, wdyt? > > renamed to resolvedClassMembers https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:63: var enclosingClass = element.enclosingElement; Sorry. If I was to write this code, I would do the following: 1) I'll drop resolveClass (now resolveMember) method altogether; 2) in assembleProgram instead of if (!element.isTopLevel()) I'd write: if (element.isMember()) { addMemberToClass(element, element.enclodingElement); return; } where addMemberToClass is (IMHO of course) better name for addResolvedElementForClass (which rather reads take unresolved class element, resolve it and add somewhere). On 2012/07/10 10:45:15, Roman wrote: > On 2012/07/10 09:50:12, antonmuhin wrote: > > it should probably be if (element.isMember()) > > what do you mean by 'it'? This is a check whether enclosing element is a class. > It may be another element kind in theory (another method maybe?), we can't > process this case yet. https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:38: * Adds given class element with its inner element to resolved classes inner should probably be something like member, it's not obvious what inner means in this context. https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:44: List<Element> resolvedElementsInClass = resolvedClassMembers.putIfAbsent( nit: even as rCM.putIfAbsent(classElement, () => <Element>[] /* no need in new... */).add(element) https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:53: void resolveMember(Element element) { again, it's not resolving. If anything, it's processing. https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:56: assert(element.isMember()); I don't think you may have element.isMember() && element.isTopLevel(), so one of those asserts is bogus.
https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:63: var enclosingClass = element.enclosingElement; On 2012/07/10 11:31:40, antonmuhin wrote: > Sorry. If I was to write this code, I would do the following: > > 1) I'll drop resolveClass (now resolveMember) method altogether; > 2) in assembleProgram instead of if (!element.isTopLevel()) I'd write: > > if (element.isMember()) { > addMemberToClass(element, element.enclodingElement); > return; > } > > where addMemberToClass is (IMHO of course) better name for > addResolvedElementForClass (which rather reads take unresolved class element, > resolve it and add somewhere). > > On 2012/07/10 10:45:15, Roman wrote: > > On 2012/07/10 09:50:12, antonmuhin wrote: > > > it should probably be if (element.isMember()) > > > > what do you mean by 'it'? This is a check whether enclosing element is a > class. > > It may be another element kind in theory (another method maybe?), we can't > > process this case yet. > Removed processMember(), moved code to assembleProgram https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:38: * Adds given class element with its inner element to resolved classes On 2012/07/10 11:31:40, antonmuhin wrote: > inner should probably be something like member, it's not obvious what inner > means in this context. Done. https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:44: List<Element> resolvedElementsInClass = resolvedClassMembers.putIfAbsent( On 2012/07/10 11:31:40, antonmuhin wrote: > nit: even as > > rCM.putIfAbsent(classElement, () => <Element>[] /* no need in new... > */).add(element) Done. https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:53: void resolveMember(Element element) { On 2012/07/10 11:31:40, antonmuhin wrote: > again, it's not resolving. If anything, it's processing. Done. https://chromiumcodereview.appspot.com/10690110/diff/7001/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:56: assert(element.isMember()); On 2012/07/10 11:31:40, antonmuhin wrote: > I don't think you may have element.isMember() && element.isTopLevel(), so one of > those asserts is bogus. removed isTopLevel() assert
stv! https://chromiumcodereview.appspot.com/10690110/diff/1003/lib/compiler/implem... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/1003/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:91: assert(enclosingClass.isTopLevel()); you may want to add an assert that it's a class.
https://chromiumcodereview.appspot.com/10690110/diff/1003/lib/compiler/implem... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10690110/diff/1003/lib/compiler/implem... lib/compiler/implementation/dart_backend/backend.dart:91: assert(enclosingClass.isTopLevel()); On 2012/07/10 12:07:29, antonmuhin wrote: > you may want to add an assert that it's a class. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
