ClassMirror.mixin

More reflection of mixin applications in the VM.

*Provide ClassMirror.mixin
*Hide extra class on the inheritance chain introduced in mixin typedefs
*Give mixin applications a simpleName that matches the source language syntax
*Update mixin_test to distinguish between members and constructors
*Update mixin_test for the mixin application naming scheme

BUG=http://dartbug.com/12464
R=ahe@google.com, asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26824

[rmacnak, 2013-08-15 22:29:06]

Regis,

Currently, a typedef mixin application introduces an extra class in the
inheritance chain. For instance,

class I {}
class S<A> {}
class M<B> {}
typedef MA<T> = S<T> with M<int> implements I

gives

MA  (super=S&M,    interfaces=[I], mixin=null)
S&M (super=S,      interfaces=[M], mixin=M)
S   (super=Object, interfaces=[],  mixin=null)

but it should give

MA  (super=S,      interfaces=[M, I], mixin=M)
S   (super=Object, interfaces=[],     mixin=null)

Fixing this invalidates two assumptions in CloneTypeParameters:
--that mixin applications don't have any type parameters
--that mixin applications have an interface list with exactly one element which
is the mixin

I think the way to address this is to
--copy down type parameters from the mixin application at (the end?) of the new
parameters list
--when changing the type arguments of the interfaces, assert that the first
entry in the interface list is the same class as the class being mixed in.

Does this make sense?

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1237: if (mixapp_class.NumTypeParameters() != 0) {
This isn't right.

[regis, 2013-08-15 22:51:48]

Just preliminary comments.
I need to think about this.
You are changing quite a few invariants...

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1239: // paramaters.
parameters

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1240: return;
Mmm, nothing to clone? I have to think about this.

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:4193: if (CurrentToken() != Token::kCOMMA &&
!final_mixin_application.IsNull()) {
Parenthesis, please.

https://codereview.chromium.org/23190003/diff/15001/tests/language/language.s...
File tests/language/language.status (right):

https://codereview.chromium.org/23190003/diff/15001/tests/language/language.s...
tests/language/language.status:75: #mixin_type_parameter2_test: Fail # Issue
11888
Should be removed if passing, not just commented out.

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
File tests/language/mixin_type_parameters_errors_test.dart (right):

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
tests/language/mixin_type_parameters_errors_test.dart:9: class B<U, V> extends S
with M<U, V> { }  /* -- this is fine */
It should be at least a static warning.

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
tests/language/mixin_type_parameters_errors_test.dart:10: class C<A, B> extends
S<A, int> with M { }  /* -- this is fine */
ditto

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
tests/language/mixin_type_parameters_errors_test.dart:13: typedef G = S<int>
with M<double, double>;  /* -- this is fine */
ditto

[ahe, 2013-08-15 22:56:58]

Everything outside runtime LGTM, as long as you remember to keep dart2js
covered.

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
File tests/language/mixin_type_parameters_errors_test.dart (right):

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
tests/language/mixin_type_parameters_errors_test.dart:9: class B<U, V> extends S
with M<U, V> { }  /* -- this is fine */
Please isolate the problematic part:

class B<U, V> extends S with M<U
, V /// 01: static warning
> { }

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
tests/language/mixin_type_parameters_errors_test.dart:10: class C<A, B> extends
S<A, int> with M { }  /* -- this is fine */
Ditto.

https://codereview.chromium.org/23190003/diff/15001/tests/language/mixin_type...
tests/language/mixin_type_parameters_errors_test.dart:13: typedef G = S<int>
with M<double, double>;  /* -- this is fine */
Ditto.

https://codereview.chromium.org/23190003/diff/15001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/23190003/diff/15001/tests/lib/lib.status#newc...
tests/lib/lib.status:20: mirrors/mixin_test: Fail # TODO(ahe): This test is
slightly broken. http://dartbug.com/12464
As discussed, please do something to avoid losing test coverage on dart2js.

https://codereview.chromium.org/23190003/diff/15001/tests/lib/lib.status#newc...
tests/lib/lib.status:21: mirrors/mixin_members_test : Fail # Issue 11863
I believe I fixed this.

https://codereview.chromium.org/23190003/diff/15001/tests/lib/mirrors/mixin_t...
File tests/lib/mirrors/mixin_test.dart (right):

https://codereview.chromium.org/23190003/diff/15001/tests/lib/mirrors/mixin_t...
tests/lib/mirrors/mixin_test.dart:134: '{test.model.C with
test.mixin_test.Mixin:'
Yikes :-)

[rmacnak, 2013-08-16 01:29:29]

On 2013/08/15 22:51:48, regis wrote:
> Just preliminary comments.
> I need to think about this.
> You are changing quite a few invariants...
> 

Just to clarify, this CL doesn't implement the changes to finalization I've
described. It's just where I got before hitting that wall. Code formatting /
test statuses will be updated once it is implemented.

[regis, 2013-08-16 01:37:33]

On 2013/08/16 01:29:29, Ryan Macnak wrote:
> On 2013/08/15 22:51:48, regis wrote:
> > Just preliminary comments.
> > I need to think about this.
> > You are changing quite a few invariants...
> > 
> 
> Just to clarify, this CL doesn't implement the changes to finalization I've
> described. It's just where I got before hitting that wall. Code formatting /
> test statuses will be updated once it is implemented.

Let's discuss in person on Friday.

[regis, 2013-08-20 18:56:37]

Ryan,

Here are a few more comments.
Astonishingly, returning immediately from ClassFinalizer::CloneTypeParameters in
case the mixin_class is an alias is the right thing to do (I think). See my
comments and try it :-)

Cheers,
Regis

https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:208: }
Can you add a comment here? Why don't you set the name if cls is a mixin?

https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:382: }
That answers my question about the missing name in CreateClassMirror, but a
comment there would still be helpful.

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:4166: const Class& final_mixin_application) {
'final_mixin_application' is not an optimal name, since 'final' has some other
meaning. How about 'mixin_application_typedef' or 'mixin_application_alias'?

https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1303: return;
I think that returning here is actually correct if the mixapp_class is an alias,
not just if it has no type parameters. Take the following example:
typedef MA = List<int> with Map<int, int>;

There is no need to parameterize MA.
Assumming List<E> and Map<K, V>, we would get MA<E, K, V>, but this is
unnecessary, since mixin sets all type parameters to 'int'.

On the other hand:
typedef MA<K> = List<int> with Map<K, int>;
MA already has one type parameter and nothing needs to change. You can return
too.

In the non-alias case:
class MA<K> extends List<int> with Map<K, int> {}
We create a S&M<E`, K, V>, which is overkill, but correct.
We could optimize this at a later stage an create a simpler S&M<K>.

Bottom line:
You need to detect if the mixapp_class is an alias. One way is to check that its
name does not contain '&'. There may be a more elegant way.

https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3804: ParseInterfaceList(mixin_application);
At this point, the mixin_application alias already has an interface, but 
ParseInterfaceList should be able to add to the list. Add a comment here.

[rmacnak, 2013-08-21 01:51:00]

Updated per comments.

I think something more needs to be done for mixin application typedefs, because
mixin_type_parameters_mixin_test is now failing.

On 2013/08/20 18:56:37, regis wrote:
> Ryan,
> 
> Here are a few more comments.
> Astonishingly, returning immediately from ClassFinalizer::CloneTypeParameters
in
> case the mixin_class is an alias is the right thing to do (I think). See my
> comments and try it :-)
> 
> Cheers,
> Regis
> 
> https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc
> File runtime/lib/mirrors.cc (right):
> 
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:208: }
> Can you add a comment here? Why don't you set the name if cls is a mixin?
> 
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
> File runtime/lib/mirrors_impl.dart (right):
> 
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
> runtime/lib/mirrors_impl.dart:382: }
> That answers my question about the missing name in CreateClassMirror, but a
> comment there would still be helpful.
> 
> https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc#newc...
> runtime/vm/parser.cc:4166: const Class& final_mixin_application) {
> 'final_mixin_application' is not an optimal name, since 'final' has some other
> meaning. How about 'mixin_application_typedef' or 'mixin_application_alias'?
> 
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
> File runtime/vm/class_finalizer.cc (right):
> 
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
> runtime/vm/class_finalizer.cc:1303: return;
> I think that returning here is actually correct if the mixapp_class is an
alias,
> not just if it has no type parameters. Take the following example:
> typedef MA = List<int> with Map<int, int>;
> 
> There is no need to parameterize MA.
> Assumming List<E> and Map<K, V>, we would get MA<E, K, V>, but this is
> unnecessary, since mixin sets all type parameters to 'int'.
> 
> On the other hand:
> typedef MA<K> = List<int> with Map<K, int>;
> MA already has one type parameter and nothing needs to change. You can return
> too.
> 
> In the non-alias case:
> class MA<K> extends List<int> with Map<K, int> {}
> We create a S&M<E`, K, V>, which is overkill, but correct.
> We could optimize this at a later stage an create a simpler S&M<K>.
> 
> Bottom line:
> You need to detect if the mixapp_class is an alias. One way is to check that
its
> name does not contain '&'. There may be a more elegant way.
> 
> https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc#newc...
> runtime/vm/parser.cc:3804: ParseInterfaceList(mixin_application);
> At this point, the mixin_application alias already has an interface, but 
> ParseInterfaceList should be able to add to the list. Add a comment here.

[regis, 2013-08-21 16:46:43]

On 2013/08/21 01:51:00, Ryan Macnak wrote:
> Updated per comments.
> 
> I think something more needs to be done for mixin application typedefs,
because
> mixin_type_parameters_mixin_test is now failing.
> 
> On 2013/08/20 18:56:37, regis wrote:
> > Ryan,
> > 
> > Here are a few more comments.
> > Astonishingly, returning immediately from
ClassFinalizer::CloneTypeParameters
> in
> > case the mixin_class is an alias is the right thing to do (I think). See my
> > comments and try it :-)
> > 
> > Cheers,
> > Regis
> > 
> > https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc
> > File runtime/lib/mirrors.cc (right):
> > 
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc#ne...
> > runtime/lib/mirrors.cc:208: }
> > Can you add a comment here? Why don't you set the name if cls is a mixin?
> > 
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
> > File runtime/lib/mirrors_impl.dart (right):
> > 
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
> > runtime/lib/mirrors_impl.dart:382: }
> > That answers my question about the missing name in CreateClassMirror, but a
> > comment there would still be helpful.
> > 
> > https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc
> > File runtime/vm/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc#newc...
> > runtime/vm/parser.cc:4166: const Class& final_mixin_application) {
> > 'final_mixin_application' is not an optimal name, since 'final' has some
other
> > meaning. How about 'mixin_application_typedef' or 'mixin_application_alias'?
> > 
> >
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
> > File runtime/vm/class_finalizer.cc (right):
> > 
> >
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
> > runtime/vm/class_finalizer.cc:1303: return;
> > I think that returning here is actually correct if the mixapp_class is an
> alias,
> > not just if it has no type parameters. Take the following example:
> > typedef MA = List<int> with Map<int, int>;
> > 
> > There is no need to parameterize MA.
> > Assumming List<E> and Map<K, V>, we would get MA<E, K, V>, but this is
> > unnecessary, since mixin sets all type parameters to 'int'.
> > 
> > On the other hand:
> > typedef MA<K> = List<int> with Map<K, int>;
> > MA already has one type parameter and nothing needs to change. You can
return
> > too.
> > 
> > In the non-alias case:
> > class MA<K> extends List<int> with Map<K, int> {}
> > We create a S&M<E`, K, V>, which is overkill, but correct.
> > We could optimize this at a later stage an create a simpler S&M<K>.
> > 
> > Bottom line:
> > You need to detect if the mixapp_class is an alias. One way is to check that
> its
> > name does not contain '&'. There may be a more elegant way.
> > 
> > https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc
> > File runtime/vm/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc#newc...
> > runtime/vm/parser.cc:3804: ParseInterfaceList(mixin_application);
> > At this point, the mixin_application alias already has an interface, but 
> > ParseInterfaceList should be able to add to the list. Add a comment here.

I'll try to reproduce the issue with mixin_type_parameters_mixin_test after
patching your cl later today.

[regis, 2013-08-21 20:25:29]

On 2013/08/21 16:46:43, regis wrote:
> On 2013/08/21 01:51:00, Ryan Macnak wrote:
> > Updated per comments.
> > 
> > I think something more needs to be done for mixin application typedefs,
> because
> > mixin_type_parameters_mixin_test is now failing.
> > 
> > On 2013/08/20 18:56:37, regis wrote:
> > > Ryan,
> > > 
> > > Here are a few more comments.
> > > Astonishingly, returning immediately from
> ClassFinalizer::CloneTypeParameters
> > in
> > > case the mixin_class is an alias is the right thing to do (I think). See
my
> > > comments and try it :-)
> > > 
> > > Cheers,
> > > Regis
> > > 
> > > https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc
> > > File runtime/lib/mirrors.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors.cc#ne...
> > > runtime/lib/mirrors.cc:208: }
> > > Can you add a comment here? Why don't you set the name if cls is a mixin?
> > > 
> > >
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
> > > File runtime/lib/mirrors_impl.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/lib/mirrors_impl....
> > > runtime/lib/mirrors_impl.dart:382: }
> > > That answers my question about the missing name in CreateClassMirror, but
a
> > > comment there would still be helpful.
> > > 
> > > https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc
> > > File runtime/vm/parser.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23190003/diff/15001/runtime/vm/parser.cc#newc...
> > > runtime/vm/parser.cc:4166: const Class& final_mixin_application) {
> > > 'final_mixin_application' is not an optimal name, since 'final' has some
> other
> > > meaning. How about 'mixin_application_typedef' or
'mixin_application_alias'?
> > > 
> > >
> >
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
> > > File runtime/vm/class_finalizer.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/class_finalize...
> > > runtime/vm/class_finalizer.cc:1303: return;
> > > I think that returning here is actually correct if the mixapp_class is an
> > alias,
> > > not just if it has no type parameters. Take the following example:
> > > typedef MA = List<int> with Map<int, int>;
> > > 
> > > There is no need to parameterize MA.
> > > Assumming List<E> and Map<K, V>, we would get MA<E, K, V>, but this is
> > > unnecessary, since mixin sets all type parameters to 'int'.
> > > 
> > > On the other hand:
> > > typedef MA<K> = List<int> with Map<K, int>;
> > > MA already has one type parameter and nothing needs to change. You can
> return
> > > too.
> > > 
> > > In the non-alias case:
> > > class MA<K> extends List<int> with Map<K, int> {}
> > > We create a S&M<E`, K, V>, which is overkill, but correct.
> > > We could optimize this at a later stage an create a simpler S&M<K>.
> > > 
> > > Bottom line:
> > > You need to detect if the mixapp_class is an alias. One way is to check
that
> > its
> > > name does not contain '&'. There may be a more elegant way.
> > > 
> > > https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc
> > > File runtime/vm/parser.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23190003/diff/28001/runtime/vm/parser.cc#newc...
> > > runtime/vm/parser.cc:3804: ParseInterfaceList(mixin_application);
> > > At this point, the mixin_application alias already has an interface, but 
> > > ParseInterfaceList should be able to add to the list. Add a comment here.
> 
> I'll try to reproduce the issue with mixin_type_parameters_mixin_test after
> patching your cl later today.

Forget my earlier explanation, which was too optimistic and definitely wrong.
I am afraid we cannot eliminate the extra class introduced by typedefs.
This example should make it clear:
class S<T> { }
class M<T> { }
class N<T> { }
typedef C<K, V, T> = S<List<V>> with M<HashMap<K, V>>, N implements I<T>;

[rmacnak, 2013-08-21 23:24:16]

Backed-out of removing the extra class introduced on mixin typedefs; having the
mirrors lie instead.

+Siva for addition of is_mixin_typedef bit to Class.

https://codereview.chromium.org/23190003/diff/30002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23190003/diff/30002/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3813: // At this point, the mixin_application alias already
has an interface, but
Is this still true with the extra class?

[siva, 2013-08-26 04:30:26]

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:213: if (Type::Handle(cls.mixin()).IsNull() ||
cls.is_mixin_typedef()) {
if ((cls.mixin() == Type::null()) || cls.is_mixin_typedef()) {

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:453: get _computeMixinApplicationName {
seems weird to have a getter named _computeMixinApplicationName,
should it just be
get _mixinApplicationName

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:484: Map<Symbol, Mirror> _members;
What does the Dart style guide say regarding listing all the instance variables
in one place, we do that for C++ code.

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:740: Symbol get simpleName => _simpleName;
do we need _simpleName and instead just have
Symbol simpleName;

and this(simpleName) above.

https://codereview.chromium.org/23190003/diff/56001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23190003/diff/56001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3845: // type parameters
Can an issue be opened for this and the TODO changed to TODO(issue#)

[rmacnak, 2013-08-26 18:24:37]

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:213: if (Type::Handle(cls.mixin()).IsNull() ||
cls.is_mixin_typedef()) {
On 2013/08/26 04:30:27, siva wrote:
> if ((cls.mixin() == Type::null()) || cls.is_mixin_typedef()) {

Done.

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:453: get _computeMixinApplicationName {
On 2013/08/26 04:30:27, siva wrote:
> seems weird to have a getter named _computeMixinApplicationName,
> should it just be
> get _mixinApplicationName

Done.

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:484: Map<Symbol, Mirror> _members;
On 2013/08/26 04:30:27, siva wrote:
> What does the Dart style guide say regarding listing all the instance
variables
> in one place, we do that for C++ code.

The style guide doesn't mention this. This file generally keeps lazy fields and
their getters together, but I wouldn't be opposed to hoisting all the fields
together.

https://codereview.chromium.org/23190003/diff/56001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:740: Symbol get simpleName => _simpleName;
On 2013/08/26 04:30:27, siva wrote:
> do we need _simpleName and instead just have
> Symbol simpleName;
> 
> and this(simpleName) above.

MethodMirror mutates this field.

https://codereview.chromium.org/23190003/diff/56001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23190003/diff/56001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3845: // type parameters
On 2013/08/26 04:30:27, siva wrote:
> Can an issue be opened for this and the TODO changed to TODO(issue#)

Done.

[siva, 2013-08-27 00:23:13]

lgtm

[rmacnak, 2013-08-28 22:56:00]

Committed patchset #9 manually as r26824 (presubmit successful).