Add a MirrorReference reflectee to MethodMirrors. Name getter now refers to

Add a MirrorReference reflectee to MethodMirrors. Name getter now refers to
internal function object, providing a name for for all cases and not only for
the ones a name has been provided in the constructor.

Fixes http://dartbug.com/6335

R=asiva@google.com, rmacnak@google.com

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

[rmacnak, 2013-07-09 22:27:46]

LGTM with comment

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1087: // TODO(turnidge): Why not use the real function
name here?
Comment no longer applies

[siva, 2013-07-10 00:09:49]

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1437: }
I think it is possible to abstract this into a single native call which returns
the name of the mirror object, i.e. if we get a class mirror object we do the
stuff in ClassMirror_name or if it is a function we do the stuff you just added.
I am sure there will be more such types.

Maybe Ryan can do this clean up to have a unified native method for getting the
name of a mirror object.

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1439: 
extra blank line here.

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:854: }
I am wondering if this can be added to the base class _LocalMirrorImpl so that
we don't have to keep repeating it every time we do this lazy setting up of
name.

[rmacnak, 2013-07-10 00:28:58]

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1437: }
On 2013/07/10 00:09:49, siva wrote:
> I think it is possible to abstract this into a single native call which
returns
> the name of the mirror object, i.e. if we get a class mirror object we do the
> stuff in ClassMirror_name or if it is a function we do the stuff you just
added.
> I am sure there will be more such types.
> 
> Maybe Ryan can do this clean up to have a unified native method for getting
the
> name of a mirror object.

So you're suggesting we dispatch on the kind of reflectee inside the native
(fewer, larger), rather than have a native for each which assumes a particular
kind (more, smaller). My inclination is toward the latter.

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:854: }
On 2013/07/10 00:09:49, siva wrote:
> I am wondering if this can be added to the base class _LocalMirrorImpl so that
> we don't have to keep repeating it every time we do this lazy setting up of
> name.

Not all mirrors have a simpleName. If we go with aggregating the natives, we
could move it to the DeclarationMirrorImpl, but currently no such beast exists.

[Michael Lippautz (Google), 2013-07-10 00:30:41]

See Ryan's remarks for the other comments.

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1439: 
On 2013/07/10 00:09:49, siva wrote:
> extra blank line here.

Done.

[siva, 2013-07-10 01:04:19]

LGTM, we can do the abstractions in a new CL.

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1437: }
Yes. It is not going to be too large maybe a switch statement based on the class
id of the reflectee. We are already into a native method the performance
overhead of a switch statement is not going to make it any worse.

With the latter approach we could end up with a number of these name methods
which almost all look the same except for the type.

On 2013/07/10 00:28:58, Ryan Macnak wrote:
> On 2013/07/10 00:09:49, siva wrote:
> > I think it is possible to abstract this into a single native call which
> returns
> > the name of the mirror object, i.e. if we get a class mirror object we do
the
> > stuff in ClassMirror_name or if it is a function we do the stuff you just
> added.
> > I am sure there will be more such types.
> > 
> > Maybe Ryan can do this clean up to have a unified native method for getting
> the
> > name of a mirror object.
> 
> So you're suggesting we dispatch on the kind of reflectee inside the native
> (fewer, larger), rather than have a native for each which assumes a particular
> kind (more, smaller). My inclination is toward the latter.

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

https://codereview.chromium.org/18473005/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:854: }
Ok when you add that class move this into that class.

On 2013/07/10 00:28:58, Ryan Macnak wrote:
> On 2013/07/10 00:09:49, siva wrote:
> > I am wondering if this can be added to the base class _LocalMirrorImpl so
that
> > we don't have to keep repeating it every time we do this lazy setting up of
> > name.
> 
> Not all mirrors have a simpleName. If we go with aggregating the natives, we
> could move it to the DeclarationMirrorImpl, but currently no such beast
exists.

[Michael Lippautz (Google), 2013-07-10 01:08:26]

Committed patchset #2 manually as r24872 (presubmit successful).