Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10692040:
Mirrors prototype added to dartdoc. (Closed)
|
Created:
8 years, 5 months ago by Johnni Winther Modified:
8 years, 5 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMirrors prototype added to dartdoc.
The mirrors system is local to dartdoc and will merged with the VM version at a later point.
TEST=compiler/dart2js/mirrors_test.dart
Committed: https://code.google.com/p/dart/source/detail?r=9406
Patch Set 1 #
Total comments: 13
Patch Set 2 : dart2js_mirror.dart included in the cl #
Total comments: 97
Patch Set 3 : Utility libraries added #
Total comments: 79
Patch Set 4 : Fixed cf. lrn's comments #
Messages
Total messages: 7 (0 generated)
The first version of the compile-time mirror system is added to dartdoc. It is not currently used by dartdoc but will serve as a basis for converting dartdoc from frog to dart2js.
lgtm with comments https://chromiumcodereview.appspot.com/10692040/diff/1/lib/dartdoc/mirrors/mi... File lib/dartdoc/mirrors/mirrors.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/1/lib/dartdoc/mirrors/mi... lib/dartdoc/mirrors/mirrors.dart:4: All my comments are already at: https://docs.google.com/a/google.com/document/d/13qt3xaBJeA2a7kVMX7HrVDkLqNFq... https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... File tests/compiler/dart2js/mirrors_helper.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_helper.dart:32: } Operator negate is going away. Is it necessary to have it in a test at this stage? https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... File tests/compiler/dart2js/mirrors_test.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:110: Expect.isFalse(objectType.isDeclaration, "Object type is declaration"); Interesting. You make a distinction between a type reference and a type declaration. This distinction only makes sense statically. This is NOT the distinction we wanted to make, which applies to generics vs parameterized types, and will in the future apply to mixin applications vs the original class. I expected this to be true. I think this is a real problem. https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:112: "Class is not subclass of superclass"); And this is inconsistent with the previous line. If objectType is not a declaration, how can it have subclasses? https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:179: "Class is not subclass of superclass"); Same issues as above https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:220: void testBaz(LibraryMirror helperLibrary, Map<Object,TypeMirror> types) { It would be helpful to embed the tested code in a comment inside these tests. https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:252: Expect.isTrue(containsType(bazClass, objectType.computeSubdeclarations()), use vs declaration issue again https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:269: Expect.isFalse(barInterface.isDeclaration, "Interface type is declaration"); use vs declaration issue again https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:294: Expect.isFalse(bazEbound.isDeclaration, "Bound is declaration"); use vs declaration issue again https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:305: Expect.isFalse(bazFbound.isDeclaration, "Bound is declaration"); use vs declaration issue again https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:363: "Unexpected parameter qualifiedName"); So where did # come from in the qualified name? https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:440: // void method3(E func(F f), Func<E,F> func) {} func should be func1 https://chromiumcodereview.appspot.com/10692040/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/mirrors_test.dart:508: "Non-empty interfaces map on typedef"); So now I'm not so sure anymore. A typedef is an alias, and maybe it should behave more like the thing it is aliasing
PTAL
Lots of style comments. Haven't tried to actually understand everything yet. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... File lib/dartdoc/mirrors/dart2js_mirror.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:36: { Brace on previous line. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:38: var link = signature.requiredParameters; Write the type of the variable. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:53: Dart2jsTypeMirror _typeConverter(Dart2jsMirrorSystem system, Seems like it should be called _convertType? I.e., an activitiy, not a noun. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:54: Type type, indentation. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:57: { Brace on previous line. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:76: Collection<Dart2jsMemberMirror> _memberConverter(Dart2jsObjectMirror library, _convertMember? Or even better, say something about what the conversion is: convertElementMembersToMirrors. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:78: { Brace on previous line. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:101: MethodMirror _methodConverter(Dart2jsObjectMirror library, Element element) { _convertMethod? https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:105: // TODO(johnniwinther): How to wrap typedefs? How is a typedef a method? https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:122: const Dart2jsMethodKind(this.text); Make a toString() for debugging. Otherwise someone debugging can't print the kind and see anything but "[instance of Dart2jsMethodKind]". https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:180: bool hasNext() => _link != null && _link.head != null; An empty linked list is "const EmptyLink<A>()". It shouldn't be represented by null, ever! So bool hasNext() => !_link.isEmpty(); https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:219: { Brace on previous line (and ditto for all later instances of this). https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:223: if (isAborting) return; Move this above "fatal" declaration. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:227: if (uri === null) { move "if (!fatal) return" in here. I recommend keeping the return on a line by itself. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:235: throw message; Put "throw message" at the end of both of the prior blocks, instead of having a seemingly inrelated if here. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:267: // interface extensions What does that mean? Capitalize/Title-case the header. For block-quotes we usually use either /************************************************* * Text here * *************************************************/ or //------------------------------------------------- // Text here //------------------------------------------------- https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:317: = const Dart2jsDiagnosticListener(); Why not make this static? Seems like a waste of space to put the same constant on all instances. If you need an instance field, make a getter that returns the const value. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:320: Map<LibraryElement,Dart2jsLibraryMirror> _libraryMap; Space after comma. Ditto all other cases of this. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:354: Iterable<InterfaceMirror> computeSubdeclarations(InterfaceMirror type) { I'm not absolutely convinced that we should have this functionality in the generic mirror system. If you don't refer to a type by name, and you don't have an instance of it, I'm not sure you should be able to know that it exists (or tree-shaking would become very problematic). I can see the need for it in a dartdoc-tool, though. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:358: _libraries.forEach((_,library) { Space after comma here too. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:370: in otherType.interfaces().getValues()) Interesting indentation. Consider using a helper variable to shorten the expression. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:371: { Brace on precious line. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:372: superInterface = superInterface.declaration; What is the difference between an interface and its declaration? https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:409: * Returns the library name (as defined by the #library tag) or for script Parenthesized comments are different in nature, which makes the sentence hard to follow. How about: "Returns the library name (for libraries with a #library tag) or the script file name (for scripts without a #library tag)." https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:411: * to private 'library name' for scripts to use for instance in dartdoc. "to private"? Something missing? https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:418: var path = _library.script.uri.path; Add typename for "path". I'm guessing "String", but aren't sure. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:431: if (e.isClass()) { Isn't isClass a getter? And isTypedef below? (And if they aren't, why not?!?) https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:441: = new ImmutableMapWrapper<Object,InterfaceMirror>(_types); '=' on previous line. Ditto in other places. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:449: if (!(e.isClass() || e.isTypedef())) { A little hard to read. Consider whether if (!e.isClass() && !e.isTypedef()) { is easier. And they are getters? https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:456: = new ImmutableMapWrapper<Object,MemberMirror>(_members); Are you sure we want to cache the wrapper, and not just create a new one each time it's needed? https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:503: final VariableElement _variableElement; Don't store this twice, just make a getter for _element: VariableElement get _variableElement() => _element; https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:524: String defaultValue() => null; // TODO(johnniwinther): How to compute this? The return type seems odd. Shouldn't this return an ObjectMirror mirroring the compile-time constant value? https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:539: final ClassElement _class; Again, don't duplicate the superclass field, just cast it in a getter. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:677: //print('$this == $other'); Remove debug codem here and below. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:707: : this._library = system.getLibrary(_typedef.getLibrary()), indent by 4. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:713: : this._library = library, By 4. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:763: });*/ Don't include commented-out code in CL. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:850: _typeVariableType.element.bound, indentation. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:953: system.compiler.dynamicClass.computeType(system.compiler))); indentation. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:1006: assert (_functionSignature !== null); indent by 2. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:1089: : _voidType = voidType, Indent by 4 (ditto in more places below, and probably above). https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... File lib/dartdoc/mirrors/mirrors.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:60: * Common interface for a type and library. ... "for types, instances and libraries." https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:94: interface TypeMirror extends Mirror { Make a comment that not all TypeMirrors are ObjectMirrors, which one would think from the comment in line 60. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:282: * Returns true if this is a top level member, i.e. a member not within a I'd say "Is true if ..." since this is a property, not a function, or, preferably, "Whether this is a ...". https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:330: * A constructor or method. State that it includes getters and setters as methods. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:425: * starts, and [:loc.source().text()[loc.end()] is where it ends. The "end" part seems odd. I don't consider the following *character* to be where something ends (a character is not a position). The position itself makes more sense. https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... File tests/compiler/dart2js/mirrors_helper.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... tests/compiler/dart2js/mirrors_helper.dart:4: Add a comment to say that this file is read from another file, so a reader won't try to make sense of this file alone. https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... File tests/compiler/dart2js/mirrors_test.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... tests/compiler/dart2js/mirrors_test.dart:9: find(Map<Object,Mirror> map, String name, [String constructorName, String operatorName]) { Line too long. Do give it a return type, since it isn't void.
Utility libraries added. Updated cf. lrn's comments. PTAL https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... File lib/dartdoc/mirrors/dart2js_mirror.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:36: { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Brace on previous line. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:38: var link = signature.requiredParameters; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Write the type of the variable. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:53: Dart2jsTypeMirror _typeConverter(Dart2jsMirrorSystem system, On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Seems like it should be called _convertType? I.e., an activitiy, not a noun. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:54: Type type, On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > indentation. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:57: { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Brace on previous line. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:76: Collection<Dart2jsMemberMirror> _memberConverter(Dart2jsObjectMirror library, On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > _convertMember? > Or even better, say something about what the conversion is: > convertElementMembersToMirrors. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:78: { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Brace on previous line. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:101: MethodMirror _methodConverter(Dart2jsObjectMirror library, Element element) { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > _convertMethod? Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:105: // TODO(johnniwinther): How to wrap typedefs? On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > How is a typedef a method? They don't seem to be. Case removed. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:122: const Dart2jsMethodKind(this.text); On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Make a toString() for debugging. Otherwise someone debugging can't print the > kind and see anything but "[instance of Dart2jsMethodKind]". Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:180: bool hasNext() => _link != null && _link.head != null; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > An empty linked list is "const EmptyLink<A>()". It shouldn't be represented by > null, ever! > So > bool hasNext() => !_link.isEmpty(); Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:223: if (isAborting) return; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Move this above "fatal" declaration. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:227: if (uri === null) { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > move "if (!fatal) return" in here. I recommend keeping the return on a line by > itself. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:235: throw message; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Put "throw message" at the end of both of the prior blocks, instead of having a > seemingly inrelated if here. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:267: // interface extensions On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > What does that mean? > Capitalize/Title-case the header. > > For block-quotes we usually use either > /************************************************* > * Text here > * > *************************************************/ > > or > > //------------------------------------------------- > // Text here > //------------------------------------------------- Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:317: = const Dart2jsDiagnosticListener(); On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Why not make this static? Seems like a waste of space to put the same constant > on all instances. > If you need an instance field, make a getter that returns the const value. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:320: Map<LibraryElement,Dart2jsLibraryMirror> _libraryMap; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Space after comma. Ditto all other cases of this. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:354: Iterable<InterfaceMirror> computeSubdeclarations(InterfaceMirror type) { I agree, I don't think it fits in the mirror system. The method will be put in dartdoc instead. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:358: _libraries.forEach((_,library) { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Space after comma here too. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:370: in otherType.interfaces().getValues()) On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Interesting indentation. Consider using a helper variable to shorten the > expression. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:371: { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Brace on precious line. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:372: superInterface = superInterface.declaration; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > What is the difference between an interface and its declaration? [:interface List<E> ...:] is the declaration whereas List<int> is an interface type instantiation of List with [int] as the type argument. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:409: * Returns the library name (as defined by the #library tag) or for script On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Parenthesized comments are different in nature, which makes the sentence hard to > follow. > How about: > "Returns the library name (for libraries with a #library tag) or the script file > name (for scripts without a #library tag)." Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:411: * to private 'library name' for scripts to use for instance in dartdoc. On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > "to private"? Something missing? Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:411: * to private 'library name' for scripts to use for instance in dartdoc. On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > "to private"? Something missing? Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:418: var path = _library.script.uri.path; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Add typename for "path". I'm guessing "String", but aren't sure. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:431: if (e.isClass()) { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Isn't isClass a getter? And isTypedef below? > (And if they aren't, why not?!?) They are methods (defined on Element). I don't know why. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:441: = new ImmutableMapWrapper<Object,InterfaceMirror>(_types); On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > '=' on previous line. Ditto in other places. The immutable map is no longer cached. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:449: if (!(e.isClass() || e.isTypedef())) { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > A little hard to read. Consider whether > if (!e.isClass() && !e.isTypedef()) { > is easier. > > And they are getters? Again, methods defined in Element. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:456: = new ImmutableMapWrapper<Object,MemberMirror>(_members); On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Are you sure we want to cache the wrapper, and not just create a new one each > time it's needed? Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:524: String defaultValue() => null; // TODO(johnniwinther): How to compute this? On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > The return type seems odd. Shouldn't this return an ObjectMirror mirroring the > compile-time constant value? Yes, it's on the general TODO for the mirror system. For now, we just need a string value for dartdoc. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:539: final ClassElement _class; On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Again, don't duplicate the superclass field, just cast it in a getter. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:677: //print('$this == $other'); On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Remove debug codem here and below. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:707: : this._library = system.getLibrary(_typedef.getLibrary()), On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > indent by 4. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:713: : this._library = library, On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > By 4. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:763: });*/ On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Don't include commented-out code in CL. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:850: _typeVariableType.element.bound, On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > indentation. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:953: system.compiler.dynamicClass.computeType(system.compiler))); On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > indentation. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:1006: assert (_functionSignature !== null); On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > indent by 2. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/dart2js_mirror.dart:1089: : _voidType = voidType, On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Indent by 4 (ditto in more places below, and probably above). Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... File lib/dartdoc/mirrors/mirrors.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:60: * Common interface for a type and library. On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > ... "for types, instances and libraries." Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:94: interface TypeMirror extends Mirror { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Make a comment that not all TypeMirrors are ObjectMirrors, which one would think > from the comment in line 60. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:282: * Returns true if this is a top level member, i.e. a member not within a On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > I'd say "Is true if ..." since this is a property, not a function, or, > preferably, "Whether this is a ...". Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:330: * A constructor or method. On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > State that it includes getters and setters as methods. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:425: * starts, and [:loc.source().text()[loc.end()] is where it ends. On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > The "end" part seems odd. I don't consider the following *character* to be where > something ends (a character is not a position). The position itself makes more > sense. I don't understand. [start] and [end] both return character positions. https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... File tests/compiler/dart2js/mirrors_helper.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... tests/compiler/dart2js/mirrors_helper.dart:4: On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Add a comment to say that this file is read from another file, so a reader won't > try to make sense of this file alone. Done. https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... File tests/compiler/dart2js/mirrors_test.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/tests/compiler/dart... tests/compiler/dart2js/mirrors_test.dart:9: find(Map<Object,Mirror> map, String name, [String constructorName, String operatorName]) { On 2012/07/02 11:48:53, Lasse Reichstein Nielsen wrote: > Line too long. Do give it a return type, since it isn't void. Done.
LGTM with suggestions. I haven't checked the logic entirely, but the semantic questions I had were all addressed in the first round. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... File lib/dartdoc/mirrors/mirrors.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:415: int start(); Make start and end getters too. Yes, I like getters better than nullary functions. https://chromiumcodereview.appspot.com/10692040/diff/7001/lib/dartdoc/mirrors... lib/dartdoc/mirrors/mirrors.dart:425: * starts, and [:loc.source().text()[loc.end()] is where it ends. Then [:end.loc():] is where it ends (and [:start.loc():] where it starts), but [:loc.source().text()[loc.end()]:] is the *character* *after* it ends - which may not even exist, if it can end at the end of the source! https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/dart2js_mirror.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:80: return [new Dart2jsFieldMirror(library, element)]; Type the literal with <Dart2jsMemberMirror>. Or perhaps only <MemberMirror> if the collection is ever exposed. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:82: return [new Dart2jsMethodMirror(library, element)]; ditto https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:100: Element element) { indent. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:125: String _getOperatorFromOperatorName(String str) { "str"->"name" It's not just a string, it's an operator name. In any case, only abbreviate a name if the abbreviation is commonly used as a word anyway (e.g., "info" for "information"). Otherwise, don't abbreviate at all. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:145: else if (str == 'or') return '|'; You could have a constant Map<String, String> to convert, and just check for null after converting. It's no less readable than this long nested if-else-if (and especially since if's with an else must always use a {...} block). I.e., Map<String, String> mapping = const { 'eq' => '==', 'not' => 'negate', // will change. 'index' => '[]', ... }; String newName = mapping[str]; if (newName === null) { throw ... } return newName; https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:152: = const Dart2jsDiagnosticListener(); Consider making it a getter instead of a field. No need to store a constant in the objhect. If not, move '=' to previous line. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:201: assert(fatal); Drop this assert. It's only checking that the 'if' statement works correctly, which I think is safe to assume. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:213: : cwd = getCurrentDirectory(), indentation. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:325: : super(system, element); indentation. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:330: Map<String, InterfaceMirror> _types; Do we really need to make these fields private? We have generally not made our implementation details private in the compiler. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:389: return new ImmutableMapWrapper<Object,MemberMirror>(_members); Space after comma. More cases below. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:459: // declarations Capitalize comment. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:544: var link = _class.interfaces; Type on "link". I assume it's Link<Something>, but I have no idea what the Something is. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:581: return new AsFilteredImmutableMapWrapper<Object, MemberMirror, MethodMirror>( Line length > 80. No, there is no way to make this pretty :( https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:631: String qualifiedName() => '${library().qualifiedName()}.${simpleName()}'; Make this, "location", "library", "typeArguments", "typeVariables", "definition", "declaredMembers", "superclass", "interfaces", "constructors" and "defaultType" getters too, if possible. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:710: { Brace on previous line (and indentation of initializer list). https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:711: assert (_typeVariableType !== null); No space after assert. It's written as a function call, not a control flow construct. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:757: print('${declarer()} != ${other.declarer()}'); Debug-print? https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:974: Extra empty line. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1034: var dollarPos = _name.indexOf('\$'); "var" => "int". No need not to. Really, don't use "var" unless the rhs is a constructor call. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1050: && _function.modifiers.isFactory()) Indent to paren. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1051: { Brace on previous line. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1108: _function.computeSignature(system.compiler)); Indentation. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1143: { Brace on previous line. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1149: : this._objectMirror = objectMirror, Indentation, both parameter and ':'. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/mirrors.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors.dart:19: { Brace on previous line. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/mirrors_util.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors_util.dart:49: * returned. If constructorName is given, but name matches a non-Method, that non-Method is returned. I.e., constructorName is ignored, even though it seems like the user had an intent with it. Is this intentional? https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors_util.dart:54: map.forEach((_,m) { space after comma, give "m" a better name and preferably a type, e.g., "(_, Mirror mirror)" (except that "mirror" is already used). We really need a find on maps. Something like: Map<K,V> { K findMatch(bool predicate(K key, V value)); Perhaps use the values collection instead: for (Mirror mirror in map.getValues()) { if (...) { return mirror; } } return null; https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors_util.dart:57: found = true; Seems convoluted, but I guess that's actually for readability. How about: if (m is! MethodMirror || ((constructorName == null || constructorName == m.constructorName) && (operatorName == null || operatorName == m.operatorName))) { mirror = m; } https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/util.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:11: * implemented immutable map. That's what the Map class itself should be! Eventually. I'd prefer to base it on a way to iterate keys. The forEach iteration can't be used lazily, like an iterator can, but that's a performance issue only. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:68: * mutable operations throw [UnsupportedOperationException] upon invocation. "mutable operations" -> "mutating operations" https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:97: class FilteredImmutableMapWrapper<K,V> extends ImmutableMapWrapper<K,V> { ImmutableValueFilterMap? I don't see the need for "Wrapper" in the name, https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:124: typedef V2 AsFilter<V1,V2>(V1 value); Space after comma, evertwhere. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:127: * An immutable map wrapper capable of filtering the input map based on types. Tricky. Might need more comments. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:129: class AsFilteredImmutableMapWrapper<K,Vin,Vout> extends AbstractMap<K,Vout> { The "filter" method don't need to do a cast. It can do an arbitrary translation to an unrelated type. You can use it for projections, e.g., with a filter Bar filter(Foo x) => x.bar; Can we specify a relation between Vin and Vout - if it's for up-casting, perhaps <K, Vin extends Vout, Vout> (possibly in a different order). https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:133: AsFilteredImmutableMapWrapper(this._map, this._filter); Indentation is one too deep from here. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:134: Should you override containsKey/containsValue to return false if filter returns null? Otherwise it won't match length(). https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... File tests/compiler/dart2js/mirrors_test.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... tests/compiler/dart2js/mirrors_test.dart:73: "Unexpected mirror type returned"); Indentation, preferably to paren. Also below. https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... tests/compiler/dart2js/mirrors_test.dart:101: Expect.isTrue(containsType(fooClass, computeSubdeclarations(system, objectType)), Line length. https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... tests/compiler/dart2js/mirrors_test.dart:117: "Class has type arguments"); Indentation still off.
https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/dart2js_mirror.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:80: return [new Dart2jsFieldMirror(library, element)]; On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Type the literal with <Dart2jsMemberMirror>. Or perhaps only <MemberMirror> if > the collection is ever exposed. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:82: return [new Dart2jsMethodMirror(library, element)]; On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > ditto Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:100: Element element) { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > indent. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:125: String _getOperatorFromOperatorName(String str) { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > "str"->"name" > It's not just a string, it's an operator name. In any case, only abbreviate a > name if the abbreviation is commonly used as a word anyway (e.g., "info" for > "information"). Otherwise, don't abbreviate at all. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:145: else if (str == 'or') return '|'; On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > You could have a constant Map<String, String> to convert, and just check for > null after converting. It's no less readable than this long nested if-else-if > (and especially since if's with an else must always use a {...} block). > I.e., > Map<String, String> mapping = const { > 'eq' => '==', > 'not' => 'negate', // will change. > 'index' => '[]', > ... > }; > String newName = mapping[str]; > if (newName === null) { > throw ... > } > return newName; Done. (Using : instead of => in the constant map!) https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:152: = const Dart2jsDiagnosticListener(); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Consider making it a getter instead of a field. No need to store a constant in > the objhect. > If not, move '=' to previous line. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:201: assert(fatal); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Drop this assert. It's only checking that the 'if' statement works correctly, > which I think is safe to assume. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:213: : cwd = getCurrentDirectory(), On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > indentation. Ignored! https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:325: : super(system, element); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > indentation. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:330: Map<String, InterfaceMirror> _types; On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Do we really need to make these fields private? > We have generally not made our implementation details private in the compiler. I like that they are private when the need initialization (by _ensureTypes()/_ensureMembers()) before use. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:389: return new ImmutableMapWrapper<Object,MemberMirror>(_members); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Space after comma. More cases below. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:459: // declarations On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Capitalize comment. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:544: var link = _class.interfaces; On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Type on "link". I assume it's Link<Something>, but I have no idea what the > Something is. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:581: return new AsFilteredImmutableMapWrapper<Object, MemberMirror, MethodMirror>( On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Line length > 80. No, there is no way to make this pretty :( Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:631: String qualifiedName() => '${library().qualifiedName()}.${simpleName()}'; On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Make this, "location", "library", "typeArguments", "typeVariables", > "definition", "declaredMembers", "superclass", "interfaces", "constructors" and > "defaultType" getters too, if possible. This issue will be considered for the whole mirrors API. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:710: { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Brace on previous line (and indentation of initializer list). Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:711: assert (_typeVariableType !== null); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > No space after assert. It's written as a function call, not a control flow > construct. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:757: print('${declarer()} != ${other.declarer()}'); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Debug-print? Yes, the one that got away! https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:974: On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Extra empty line. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1034: var dollarPos = _name.indexOf('\$'); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > "var" => "int". No need not to. > Really, don't use "var" unless the rhs is a constructor call. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1050: && _function.modifiers.isFactory()) On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Indent to paren. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1051: { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Brace on previous line. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1108: _function.computeSignature(system.compiler)); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Indentation. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1143: { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Brace on previous line. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/dart2js_mirror.dart:1149: : this._objectMirror = objectMirror, On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Indentation, both parameter and ':'. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/mirrors.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors.dart:19: { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Brace on previous line. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/mirrors_util.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors_util.dart:49: * returned. On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > If constructorName is given, but name matches a non-Method, that non-Method is > returned. I.e., constructorName is ignored, even though it seems like the user > had an intent with it. Is this intentional? It wasn't. The test has been rewritten. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors_util.dart:54: map.forEach((_,m) { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > space after comma, give "m" a better name and preferably a type, e.g., "(_, > Mirror mirror)" (except that "mirror" is already used). > > We really need a find on maps. Something like: > Map<K,V> { > K findMatch(bool predicate(K key, V value)); > > Perhaps use the values collection instead: > > for (Mirror mirror in map.getValues()) { > if (...) { > return mirror; > } > } > return null; Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/mirrors_util.dart:57: found = true; On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Seems convoluted, but I guess that's actually for readability. > How about: > if (m is! MethodMirror || > ((constructorName == null || > constructorName == m.constructorName) && > (operatorName == null || > operatorName == m.operatorName))) { > mirror = m; > } This wouldn't work for if [m] is a FieldMirror and [constructorName] is non-null. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... File lib/dartdoc/mirrors/util.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:11: * implemented immutable map. On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > That's what the Map class itself should be! > Eventually. > > I'd prefer to base it on a way to iterate keys. The forEach iteration can't be > used lazily, like an iterator can, but that's a performance issue only. Yes, an iterator is wanted/needed. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:68: * mutable operations throw [UnsupportedOperationException] upon invocation. On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > "mutable operations" -> "mutating operations" Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:124: typedef V2 AsFilter<V1,V2>(V1 value); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Space after comma, evertwhere. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:127: * An immutable map wrapper capable of filtering the input map based on types. On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Tricky. Might need more comments. Done. (Or tried!) https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:129: class AsFilteredImmutableMapWrapper<K,Vin,Vout> extends AbstractMap<K,Vout> { On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > The "filter" method don't need to do a cast. It can do an arbitrary translation > to an unrelated type. You can use it for projections, e.g., with a filter > Bar filter(Foo x) => x.bar; > Can we specify a relation between Vin and Vout - if it's for up-casting, perhaps > > <K, Vin extends Vout, Vout> > (possibly in a different order). The filter method could be used for other purposes. The map could be a candidate for a collection more general utility collections, and should in such a case be defined and described in the broadest terms. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:133: AsFilteredImmutableMapWrapper(this._map, this._filter); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Indentation is one too deep from here. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/lib/dartdoc/mirror... lib/dartdoc/mirrors/util.dart:134: On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Should you override containsKey/containsValue to return false if filter returns > null? Otherwise it won't match length(). It works with the current implementation of AbstractMap whichs uses forEach for containsKey/containsValue. https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... File tests/compiler/dart2js/mirrors_test.dart (right): https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... tests/compiler/dart2js/mirrors_test.dart:73: "Unexpected mirror type returned"); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Indentation, preferably to paren. Also below. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... tests/compiler/dart2js/mirrors_test.dart:101: Expect.isTrue(containsType(fooClass, computeSubdeclarations(system, objectType)), On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Line length. Done. https://chromiumcodereview.appspot.com/10692040/diff/10002/tests/compiler/dar... tests/compiler/dart2js/mirrors_test.dart:117: "Class has type arguments"); On 2012/07/04 10:58:59, Lasse Reichstein Nielsen wrote: > Indentation still off. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
