Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11571058:
DeclarationMirror extended with metadata. (Closed)
|
Created:
8 years ago by Johnni Winther Modified:
7 years, 11 months ago CC:
reviews_dartlang.org, turnidge, Andrei Mouravski Visibility:
Public. |
DescriptionDeclarationMirror extended with metadata.
InstanceMirror is used for metadata values.
InstanceMirrors for null, bool, num, and String constants have a reflectee.
Special InstanceMirror classes added for constant lists and maps and type constants.
DartdocComment class added to encode comments as metadata. This is not implemented yet.
Committed: https://code.google.com/p/dart/source/detail?r=16630
Patch Set 1 #
Total comments: 51
Patch Set 2 : Updated cf. comments. #
Total comments: 4
Patch Set 3 : Rebased #Patch Set 4 : Use Compiler.internalError #Patch Set 5 : Updated cf. comments #
Total comments: 2
Patch Set 6 : Updated cf. comment. #
Messages
Total messages: 13 (0 generated)
This is awesome! https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... File tests/compiler/dart2js/mirrors_helper.dart (right): https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_helper.dart:26: class Foo { For the sake of completeness, would you mind adding a @Metadata in front of "class Foo {" and also I think @Metadata can be on the same line but separated by commas.
LGTM https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:133: // TODO(johnniwinther): Introduce a special ClassMirror for the null type? Why? null is an instance of Null. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:138: throw new ArgumentError( This is an internal error as _convertConstantToInstanceMirror is private. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:688: Remove extra lines. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1173: List<InstanceMirror> get metadata { My personal opinion on style: I don't like getters that compute stuff over and over. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1617: // Mirrors on constant values used for metadata. Section comments don't work. A separate file or library does. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1621: final Dart2JsMirrorSystem mirrors; If you store the type in a final, you don't need mirrors. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1626: ClassMirror get type => new Dart2JsClassMirror( If it doesn't fit on one line, my personal preference is to not use the => short-hand. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1637: throw new NoSuchMethodError(this, fieldName, [], null); Why are you throwing NoSuchMethod? https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1665: StringConstant constant) Indentation. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1700: var completer = new Completer<InstanceMirror>(); return new Future.immediate(_convertConstantToInstanceMirror(mirrors, _constant.entries[index])); https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1719: _list = new List<String>(); Could this be a fixed-size list? https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1731: return _list; return new List<String>.from(_list) https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1736: int index = _list.indexOf(key); How about using "keys" instead. Then you don't need to call _ensureKeyList. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1738: var completer = new Completer<InstanceMirror>(); Future.immediate https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1756: Extra line. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1774: fieldConstant = _constant.fields[index]; You need to stop iterating when a match is found: class Super { int x; } class Sub extends Super { int x; } https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1779: var completer = new Completer<InstanceMirror>(); Future.immediate. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1787: Extra line. https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... File tests/compiler/dart2js/mirrors_helper.dart (right): https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_helper.dart:26: class Foo { On 2012/12/19 14:42:02, Andrei Mouravski wrote: > For the sake of completeness, would you mind adding a @Metadata in front of > "class Foo {" and also I think @Metadata can be on the same line but separated > by commas. Line breaks don't matter. Metadata cannot be separated by commas. The grammar rule is: metadata: (‘@’ qualified (. identifier)? (arguments)?)* ; The example could be written as: @Metadata@Metadata(null)@Metadata(true)@Metadata(false)@Metadata(0)@Metadata(1.5)@Metadata("Foo")@Metadata(const["Foo"])@Metadata(const{'foo':"Foo"})class Foo{} You only need one whitespace, between "class" and "Foo". https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_helper.dart:69: const Metadata(this.data); This should be an optional argument, otherwise you should be getting a compile-time error on @Metadata. https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... File tests/compiler/dart2js/mirrors_test.dart (right): https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_test.dart:170: // @Metadata These comments would be more helpful closer to the section that test them.
Personal clarification comment. https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... File tests/compiler/dart2js/mirrors_helper.dart (right): https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_helper.dart:26: class Foo { On 2012/12/19 15:44:03, ahe wrote: > On 2012/12/19 14:42:02, Andrei Mouravski wrote: > > For the sake of completeness, would you mind adding a @Metadata in front of > > "class Foo {" and also I think @Metadata can be on the same line but separated > > by commas. > > Line breaks don't matter. Metadata cannot be separated by commas. The grammar > rule is: > > metadata: > (‘@’ qualified (. identifier)? (arguments)?)* ; > > The example could be written as: > > @Metadata@Metadata(null)@Metadata(true)@Metadata(false)@Metadata(0)@Metadata(1.5)@Metadata("Foo")@Metadata(const["Foo"])@Metadata(const{'foo':"Foo"})class > Foo{} > > You only need one whitespace, between "class" and "Foo". Good to know. Could you show an example of using the identifier? That just means it calls a named constructor, right? Is that like this?: @Metadata.foo("arg") What does metadata of a compile-time constant variable look like? Is it: @1.5 Lastly, can the class that provides the metadata ("class Metadata" here) actually do any work? Is there any case where that class has a non compile time use? Would a metadata class ever have methods? Or be referred to or constructed during execution?
https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... File tests/compiler/dart2js/mirrors_helper.dart (right): https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_helper.dart:26: class Foo { On 2012/12/19 16:31:19, Andrei Mouravski wrote: > Could you show an example of using the identifier? That just means it calls a > named constructor, right? > Is that like this?: > @Metadata.foo("arg") Actually, qualified matches Metadata.foo. So it would have to be something like: @libraryPrefix.Metadata.foo("arg") > What does metadata of a compile-time constant variable look like? Is it: > @1.5 That is not legal syntax, because the rule is: metadata: (‘@’ qualified (. identifier)? (arguments)?)* ; qualified: identifier (‘.’ identifier)? ; So @ has to be followed by an identifier. > Lastly, can the class that provides the metadata ("class Metadata" here) > actually do any work? Is there any case where that class has a non compile time > use? Would a metadata class ever have methods? Or be referred to or constructed > during execution? There are only two requirements on metadata: * It must match the grammar above, * and it must be a compile-time constant. A compile-time constant can have methods. Those aren't useful in source mirrors, but in the VM, you should be able to do something like this: // Assume you have a ClassMirror on Foo. ClassMirror fooClassMirror = ...; InstanceMirror im = fooClassMirror.metadata[0]; Metadata metadata = im.reflectee; metadata.someMethod();
PTAL https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:133: // TODO(johnniwinther): Introduce a special ClassMirror for the null type? On 2012/12/19 15:44:03, ahe wrote: > Why? null is an instance of Null. Null is currently an internal class in js_helper, but I can see from a TODO on it that it is intended to be in dart:core. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:138: throw new ArgumentError( On 2012/12/19 15:44:03, ahe wrote: > This is an internal error as _convertConstantToInstanceMirror is private. Changed to InternalError (+ some more places). https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:688: On 2012/12/19 15:44:03, ahe wrote: > Remove extra lines. Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1173: List<InstanceMirror> get metadata { On 2012/12/19 15:44:03, ahe wrote: > My personal opinion on style: I don't like getters that compute stuff over and > over. I don't either. Currently dart2js_mirror generally computes too much repeatedly. Changed this to use caching. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1173: List<InstanceMirror> get metadata { On 2012/12/19 15:44:03, ahe wrote: > My personal opinion on style: I don't like getters that compute stuff over and > over. I don't either. Currently dart2js_mirror generally computes too much repeatedly. Changed metadata to use caching. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1617: // Mirrors on constant values used for metadata. On 2012/12/19 15:44:03, ahe wrote: > Section comments don't work. A separate file or library does. Will do a split into multiple files at some point. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1621: final Dart2JsMirrorSystem mirrors; On 2012/12/19 15:44:03, ahe wrote: > If you store the type in a final, you don't need mirrors. It is required by the implemented Mirror interface. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1626: ClassMirror get type => new Dart2JsClassMirror( On 2012/12/19 15:44:03, ahe wrote: > If it doesn't fit on one line, my personal preference is to not use the => > short-hand. Changed to { } https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1637: throw new NoSuchMethodError(this, fieldName, [], null); On 2012/12/19 15:44:03, ahe wrote: > Why are you throwing NoSuchMethod? Changed to UnsupportedError and added a TODO. I'm not sure what the intended behaviour is. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1665: StringConstant constant) On 2012/12/19 15:44:03, ahe wrote: > Indentation. Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1700: var completer = new Completer<InstanceMirror>(); On 2012/12/19 15:44:03, ahe wrote: > return new Future.immediate(_convertConstantToInstanceMirror(mirrors, > _constant.entries[index])); Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1719: _list = new List<String>(); On 2012/12/19 15:44:03, ahe wrote: > Could this be a fixed-size list? Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1731: return _list; On 2012/12/19 15:44:03, ahe wrote: > return new List<String>.from(_list) Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1736: int index = _list.indexOf(key); On 2012/12/19 15:44:03, ahe wrote: > How about using "keys" instead. Then you don't need to call _ensureKeyList. I don't want to copy the keys list as is now done in keys. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1738: var completer = new Completer<InstanceMirror>(); On 2012/12/19 15:44:03, ahe wrote: > Future.immediate Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1756: On 2012/12/19 15:44:03, ahe wrote: > Extra line. Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1774: fieldConstant = _constant.fields[index]; On 2012/12/19 15:44:03, ahe wrote: > You need to stop iterating when a match is found: > > class Super { int x; } > class Sub extends Super { int x; } Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1774: fieldConstant = _constant.fields[index]; On 2012/12/19 15:44:03, ahe wrote: > You need to stop iterating when a match is found: > > class Super { int x; } > class Sub extends Super { int x; } Changed to compute a field map instead. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1779: var completer = new Completer<InstanceMirror>(); On 2012/12/19 15:44:03, ahe wrote: > Future.immediate. Done. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1787: On 2012/12/19 15:44:03, ahe wrote: > Extra line. Done. https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... File tests/compiler/dart2js/mirrors_helper.dart (right): https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_helper.dart:69: const Metadata(this.data); On 2012/12/19 15:44:03, ahe wrote: > This should be an optional argument, otherwise you should be getting a > compile-time error on @Metadata. No. @Metadata is the type literal constant. It would be an error to have @Metadata() though. https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... File tests/compiler/dart2js/mirrors_test.dart (right): https://codereview.chromium.org/11571058/diff/1/tests/compiler/dart2js/mirror... tests/compiler/dart2js/mirrors_test.dart:170: // @Metadata On 2012/12/19 15:44:03, ahe wrote: > These comments would be more helpful closer to the section that test them. Done.
PTAL
Forgot to mail this out. Taking another look right now. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1621: final Dart2JsMirrorSystem mirrors; On 2012/12/20 11:30:35, Johnni Winther wrote: > On 2012/12/19 15:44:03, ahe wrote: > > If you store the type in a final, you don't need mirrors. > > It is required by the implemented Mirror interface. The Mirror interface needs to be changed, then. An extra field on each mirror is a pretty steep price to pay for something that serves no real purpose.
Still LGTM! https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1736: int index = _list.indexOf(key); On 2012/12/20 11:30:35, Johnni Winther wrote: > On 2012/12/19 15:44:03, ahe wrote: > > How about using "keys" instead. Then you don't need to call _ensureKeyList. > > I don't want to copy the keys list as is now done in keys. Sounds like you need a todo for updating this code when the library is fixed. My understanding is that this is one of the things Florian wants to fix in the collection library. On a related note: calling _ensureKeyList() before calling _list doesn't seem elegant. Why don't you integrate _ensureKeyList in a _list getter? https://codereview.chromium.org/11571058/diff/6001/sdk/lib/_internal/compiler... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/6001/sdk/lib/_internal/compiler... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1773: void _ensureFieldMap() { Inelegant cf my comment on _ensureKeyList. https://codereview.chromium.org/11571058/diff/6001/tests/compiler/dart2js/mir... File tests/compiler/dart2js/mirrors_helper.dart (right): https://codereview.chromium.org/11571058/diff/6001/tests/compiler/dart2js/mir... tests/compiler/dart2js/mirrors_helper.dart:17: @Metadata Add comment saying that you intentionally is getting the type literal here.
https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1621: final Dart2JsMirrorSystem mirrors; On 2013/01/03 12:03:05, ahe wrote: > On 2012/12/20 11:30:35, Johnni Winther wrote: > > On 2012/12/19 15:44:03, ahe wrote: > > > If you store the type in a final, you don't need mirrors. > > > > It is required by the implemented Mirror interface. > > The Mirror interface needs to be changed, then. An extra field on each mirror > is a pretty steep price to pay for something that serves no real purpose. Added a TODO. https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1736: int index = _list.indexOf(key); On 2013/01/03 12:23:02, ahe wrote: > On 2012/12/20 11:30:35, Johnni Winther wrote: > > On 2012/12/19 15:44:03, ahe wrote: > > > How about using "keys" instead. Then you don't need to call _ensureKeyList. > > > > I don't want to copy the keys list as is now done in keys. > > Sounds like you need a todo for updating this code when the library is fixed. My > understanding is that this is one of the things Florian wants to fix in the > collection library. > > On a related note: calling _ensureKeyList() before calling _list doesn't seem > elegant. Why don't you integrate _ensureKeyList in a _list getter? Changed to _ensureKeyList to _getKeyList which returns the ensured list. https://codereview.chromium.org/11571058/diff/6001/sdk/lib/_internal/compiler... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/6001/sdk/lib/_internal/compiler... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1773: void _ensureFieldMap() { On 2013/01/03 12:23:02, ahe wrote: > Inelegant cf my comment on _ensureKeyList. Changed _ensureFieldMap to _getFieldMap which returns the ensured map. https://codereview.chromium.org/11571058/diff/6001/tests/compiler/dart2js/mir... File tests/compiler/dart2js/mirrors_helper.dart (right): https://codereview.chromium.org/11571058/diff/6001/tests/compiler/dart2js/mir... tests/compiler/dart2js/mirrors_helper.dart:17: @Metadata On 2013/01/03 12:23:02, ahe wrote: > Add comment saying that you intentionally is getting the type literal here. Done.
Just debating style, you have an LGTM already :-) https://codereview.chromium.org/11571058/diff/20002/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/20002/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1726: List<String> _getKeyList() { I'm still not feeling the love for this. This feel like a Java program :-( Having a method named getFoo should mostly be replaced by a getter in Dart. I think this is cleaner: List<String> _listCache; List<String> get _list { ... }
https://codereview.chromium.org/11571058/diff/20002/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/20002/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1726: List<String> _getKeyList() { On 2013/01/04 09:20:44, ahe wrote: > I'm still not feeling the love for this. This feel like a Java program :-( > > Having a method named getFoo should mostly be replaced by a getter in Dart. I > think this is cleaner: > > List<String> _listCache; > > List<String> get _list { > ... > } Done.
Message was sent while issue was closed.
https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/11571058/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1621: final Dart2JsMirrorSystem mirrors; This is interesting collateral damage from the change from interfaces to classes. When the type was an interface, final foo was just a different way of defining a getter, and implied nothing about storage. Converting to a class changes that. So the interface should just define a getter. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
