Implementation for ClosureMirror.findInContext.

Implementation for ClosureMirror.findInContext.

Fixes issue 5897.

BUG=https://code.google.com/p/dart/issues/detail?id=5897
R=asiva@google.com, gbracha@google.com, rmacnak@google.com

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

[Michael Lippautz (Google), 2013-08-27 20:54:01]

RFC. Probably requires more refactoring. Ideas?

Code itself works as intended, accounting for imports (w/ and w/o prefixes,
hide, show), statics, globals, instance fields, and ofc fields in context.

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

https://codereview.chromium.org/23633002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1116: enum {
Although this is valid C++, I have the feeling that we do this differently.
Suggestions?

[Michael Lippautz (Google), 2013-09-05 20:26:53]

+Peter: API + tests

Rebased and reworked the complete patchset.

Partially depends on https://codereview.chromium.org/23909002/ (resolver.h/.cc
and ClosureForStaticFunction() in mirrors.cc)

[Michael Lippautz (Google), 2013-09-25 23:25:34]

PTAL

Rebased on not dependent on any other CLs anymore.

[gbracha, 2013-09-25 23:26:54]

API lgtm

[siva, 2013-09-26 17:05:07]

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:411: }
This function should probably be an instance method of
class Field.

so you could do field.IsUninitialized()....

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:456: // library or in the field's owner class, depending.
This comment seems to be incomplete....

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:472: }
I saw some similar code in Ryan's CL maybe it needs to be abstracted into a
helper function.

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:474: } else if (!field.IsNull() &&
FieldIsUninitialized(field)) {
At this point we know field.IsNull() is not true why check again?

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:493: return Instance::null();
Maybe you should have a static helper method
static RawInstance* returnResult(const Instance& result);
and move this code of checking for error, non NULL etc. over there.

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:497: }
I think the code needs to be restructured as
if (field.IsNull()) {
  ......
  ......
} else {
  if (field.IsInitialized()) {
    return field.value();
  }
  ....
  ....
}
if (!getter.IsNull() && ...) {
  ...
  ...
}

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:552: return Instance::null();
ditto comment about returnResult

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:568: if (!function.IsNull() || (function.IsNull() &&
throw_nsm_if_absent)) {
shouldn't this just be
if (!function.IsNull() || throw_nsm_if_absent) {

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:597: }
why not just
return result.raw();

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:598: } else if (!entry.IsNull() && entry.IsFunction()) {
why test of !entry.IsNull() twice?
Structure as
if (!entry.IsNull()) {
  if (entry.IsField()) {
    ....
  }
  ASSERT(entry.IsFunction()); // Can entry be anything else ?
  ....
}

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:623: func.ImplicitClosureFunction());
ASSERT(!closure_function.IsNull());

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1204: unsigned lookups = kNoLookups;
we don't normally used unsigned like this,
why not
intptr_t lookups = kNoLookups;

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1252: }
I am not sure if this 'lookups' stuff is a good idea, we end up checking twice
and in the case of global or library prefix we go through 2 additional checks.

Why not just inline the Lookup calls in the if statements above.

https://chromiumcodereview.appspot.com/23633002/diff/21001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1256: return Instance::null();
return result.raw();
is sufficient

[rmacnak, 2013-09-26 17:49:38]

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

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:411: }
On 2013/09/26 17:05:07, siva wrote:
> This function should probably be an instance method of
> class Field.
> 
> so you could do field.IsUninitialized()....

Please also update the equivalent use in Dart_SetField.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:416: static RawInstance*
ReflectivelyInvokeDynamicFunction(
Let's drop the "Reflectively" for parallelism with the other helpers.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:456: // library or in the field's owner class, depending.
On 2013/09/26 17:05:07, siva wrote:
> This comment seems to be incomplete....

To perform a reflective getter invocation, we need to check for a Field or a
Function because. Unlike Instances, the fields of Libraries are initialized
lazily, so we need to invoke the corresponding function if it is uninitialized.
IIRC, these functions cannot be found by library.LookupFunction and should not
be invoked if the field is already initialized. Presumably we have these
irregularities to optimize access to library/static fields.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:472: }
On 2013/09/26 17:05:07, siva wrote:
> I saw some similar code in Ryan's CL maybe it needs to be abstracted into a
> helper function.

That code was moved here, not duplicated.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:543: Object& result = Object::Handle(
const

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:598: } else if (!entry.IsNull() && entry.IsFunction()) {
On 2013/09/26 17:05:07, siva wrote:
> why test of !entry.IsNull() twice?
> Structure as
> if (!entry.IsNull()) {
>   if (entry.IsField()) {
>     ....
>   }
>   ASSERT(entry.IsFunction()); // Can entry be anything else ?
>   ....
> }

Could it be a Class?

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:617: Class& kls = Class::Handle(klass.raw());
Consider 'lookup_class'

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:654: // functions and fields through their internal
getter name. If the internal
implicit getter, not internal

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1238: if ((lookups & kContext) && result.IsNull()) {
I'm suspicious of using null to indicate no-result. What if one of the lookups
finds a null, which is the correct value, but the lookups proceed to outer
scopes and find a non-null shadowed value?

import 'dart:mirrors';

class C {
  static var foo = 'not-null';
  makeClosure() {
     var foo = null;
     return () => foo;
  }
}
main(){
  var m = reflect(new C().makeClosure());
  print(m.findInContext(#foo)); // Gives mirror on 'not-null', should give
mirror on null
}

[Michael Lippautz (Google), 2013-09-26 21:16:01]

Thanks for all the comments!

- Since a field being null is different from not finding a field, I introduced
Object::sentinel() as special null indicating that a field has not been found.
This null is never leaked into Dartland.

- Added the case checking for a null field hiding another field to the tests. 

- Lookups are now inlined and use the magic Object::sentinel() null to check
whether there's a need to search further.

- Tests are converted to using Symbol literals.

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

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:411: }
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> On 2013/09/26 17:05:07, siva wrote:
> > This function should probably be an instance method of
> > class Field.
> > 
> > so you could do field.IsUninitialized()....
> 
> Please also update the equivalent use in Dart_SetField.

Done.(including Dart_GetField/_SetField)

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:416: static RawInstance*
ReflectivelyInvokeDynamicFunction(
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> Let's drop the "Reflectively" for parallelism with the other helpers.

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:456: // library or in the field's owner class, depending.
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> On 2013/09/26 17:05:07, siva wrote:
> > This comment seems to be incomplete....
> 
> To perform a reflective getter invocation, we need to check for a Field or a
> Function because. Unlike Instances, the fields of Libraries are initialized
> lazily, so we need to invoke the corresponding function if it is
uninitialized.
> IIRC, these functions cannot be found by library.LookupFunction and should not
> be invoked if the field is already initialized. Presumably we have these
> irregularities to optimize access to library/static fields.

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:472: }
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> On 2013/09/26 17:05:07, siva wrote:
> > I saw some similar code in Ryan's CL maybe it needs to be abstracted into a
> > helper function.
> 
> That code was moved here, not duplicated.

This is Ryan's code. It just moved here.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:474: } else if (!field.IsNull() &&
FieldIsUninitialized(field)) {
On 2013/09/26 17:05:07, siva wrote:
> At this point we know field.IsNull() is not true why check again?

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:493: return Instance::null();
On 2013/09/26 17:05:07, siva wrote:
> Maybe you should have a static helper method
> static RawInstance* returnResult(const Instance& result);
> and move this code of checking for error, non NULL etc. over there.

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:497: }
On 2013/09/26 17:05:07, siva wrote:
> I think the code needs to be restructured as
> if (field.IsNull()) {
>   ......
>   ......
> } else {
>   if (field.IsInitialized()) {
>     return field.value();
>   }
>   ....
>   ....
> }
> if (!getter.IsNull() && ...) {
>   ...
>   ...
> }

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:543: Object& result = Object::Handle(
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> const

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:552: return Instance::null();
On 2013/09/26 17:05:07, siva wrote:
> ditto comment about returnResult

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:568: if (!function.IsNull() || (function.IsNull() &&
throw_nsm_if_absent)) {
On 2013/09/26 17:05:07, siva wrote:
> shouldn't this just be
> if (!function.IsNull() || throw_nsm_if_absent) {

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:597: }
On 2013/09/26 17:05:07, siva wrote:
> why not just
> return result.raw();

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:598: } else if (!entry.IsNull() && entry.IsFunction()) {
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> On 2013/09/26 17:05:07, siva wrote:
> > why test of !entry.IsNull() twice?
> > Structure as
> > if (!entry.IsNull()) {
> >   if (entry.IsField()) {
> >     ....
> >   }
> >   ASSERT(entry.IsFunction()); // Can entry be anything else ?
> >   ....
> > }
> 
> Could it be a Class?

Can be a class. Restructured nonetheless.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:617: Class& kls = Class::Handle(klass.raw());
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> Consider 'lookup_class'

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:623: func.ImplicitClosureFunction());
On 2013/09/26 17:05:07, siva wrote:
> ASSERT(!closure_function.IsNull());

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:654: // functions and fields through their internal
getter name. If the internal
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> implicit getter, not internal

Done.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1204: unsigned lookups = kNoLookups;
On 2013/09/26 17:05:07, siva wrote:
> we don't normally used unsigned like this,
> why not
> intptr_t lookups = kNoLookups;

Not used anymore.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1238: if ((lookups & kContext) && result.IsNull()) {
On 2013/09/26 17:49:39, Ryan Macnak wrote:
> I'm suspicious of using null to indicate no-result. What if one of the lookups
> finds a null, which is the correct value, but the lookups proceed to outer
> scopes and find a non-null shadowed value?
> 
> import 'dart:mirrors';
> 
> class C {
>   static var foo = 'not-null';
>   makeClosure() {
>      var foo = null;
>      return () => foo;
>   }
> }
> main(){
>   var m = reflect(new C().makeClosure());
>   print(m.findInContext(#foo)); // Gives mirror on 'not-null', should give
> mirror on null
> } 

Done (see above).

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1252: }
On 2013/09/26 17:05:07, siva wrote:
> I am not sure if this 'lookups' stuff is a good idea, we end up checking twice
> and in the case of global or library prefix we go through 2 additional checks.
> 
> Why not just inline the Lookup calls in the if statements above.

Inlined the lookup calls.

https://codereview.chromium.org/23633002/diff/21001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1256: return Instance::null();
On 2013/09/26 17:05:07, siva wrote:
> return result.raw();
> is sufficient

Different now.

[rmacnak, 2013-09-26 22:16:00]

LGTM with comments

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

https://codereview.chromium.org/23633002/diff/32001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:428: if (result.IsError()) {
Could also use returnResult here.

https://codereview.chromium.org/23633002/diff/32001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:504: // a special null class. This is different from a
field being null and must
special null instance. It has the same class as the user-visible null.

Likewise to the repeats of this comment.

https://codereview.chromium.org/23633002/diff/32001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1248: const Array& result_tuple =
Array::Handle(Array::New(2));
Add comment that multiple return values is to distinguish the case where we
found null and the case where we didn't find anything.

[Michael Lippautz (Google), 2013-09-26 22:40:50]

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

https://codereview.chromium.org/23633002/diff/32001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:428: if (result.IsError()) {
On 2013/09/26 22:16:00, Ryan Macnak wrote:
> Could also use returnResult here.

Done.

https://codereview.chromium.org/23633002/diff/32001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:504: // a special null class. This is different from a
field being null and must
On 2013/09/26 22:16:00, Ryan Macnak wrote:
> special null instance. It has the same class as the user-visible null.
> 
> Likewise to the repeats of this comment.

Done.

https://codereview.chromium.org/23633002/diff/32001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1248: const Array& result_tuple =
Array::Handle(Array::New(2));
On 2013/09/26 22:16:00, Ryan Macnak wrote:
> Add comment that multiple return values is to distinguish the case where we
> found null and the case where we didn't find anything.

Done.

[siva, 2013-09-27 00:21:00]

LGTM with some comments

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

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:406: } else if (result.IsInstance()) {
if (result.IsInstance()) {
  ...
  ...
}

The else is not needed as you have an UNREACHABLE();

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:425: 
blank line not needed

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:461: // Looking for a getter but found a regular method:
closurize.
closurize it.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:469: // A field was found.  Check for a getter in the
field's owner classs.
An uninitialized field was found.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:476: }
will read better if you wrote it as
if (!field.IsUninitialized()) {
  return field.value();
}
...
...
...

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:497: // not leak into Dartland.
How do you ensure that it does not leak into Dart land?

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:517: // Looking for a getter but found a regular method:
closurize.
closurize it.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:552: String& internal_getter_name =
String::Handle(Field::GetterName(getter_name));
const String&

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:554: Resolver::ResolveDynamicAnyArgsAllowPrivate(klass,
internal_getter_name));
const Function& function

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:562: 
Add a comment here that InvokeDynamicFunction invokes NoSuchMethod if function
is Null

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:579: Instance& result = Instance::Handle();
This declaration can be moved inside the if (...) as it is only used there

const Instance& result = Result::Handle(InvokeLibrary......)

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:678: InvokeLibraryGetter(library, lookup_name, false));
const Instance& result

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:682: Function& func = Function::Handle(
const Function

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:690: Class& cls = Class::Handle(
const Class

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1225: } else if (parts_len == 3) {
this can just be:

else {
  ASSERT(parts_len == 3);
  ....
}

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1240: // We return a tuple (list) where the first slot
indicates whether we found a
first slot is a boolean that indicates

[Michael Lippautz (Google), 2013-09-27 01:28:20]

Committed patchset #5 manually as r27982 (presubmit successful).

[Michael Lippautz (Google), 2013-09-27 01:29:29]

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

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:406: } else if (result.IsInstance()) {
On 2013/09/27 00:21:01, siva wrote:
> if (result.IsInstance()) {
>   ...
>   ...
> }
> 
> The else is not needed as you have an UNREACHABLE();

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:425: 
On 2013/09/27 00:21:01, siva wrote:
> blank line not needed

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:461: // Looking for a getter but found a regular method:
closurize.
On 2013/09/27 00:21:01, siva wrote:
> closurize it.

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:469: // A field was found.  Check for a getter in the
field's owner classs.
On 2013/09/27 00:21:01, siva wrote:
> An uninitialized field was found.

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:476: }
On 2013/09/27 00:21:01, siva wrote:
> will read better if you wrote it as
> if (!field.IsUninitialized()) {
>   return field.value();
> }
> ...
> ...
> ...

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:497: // not leak into Dartland.
On 2013/09/27 00:21:01, siva wrote:
> How do you ensure that it does not leak into Dart land?

Adjusted the comment.

As clarified offline: All these functions are static and thus local to the
mirrors.cc file. The calling root function (ClosureMirror_findInContext) make
sure that this special null does not leak.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:517: // Looking for a getter but found a regular method:
closurize.
On 2013/09/27 00:21:01, siva wrote:
> closurize it.

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:552: String& internal_getter_name =
String::Handle(Field::GetterName(getter_name));
On 2013/09/27 00:21:01, siva wrote:
> const String&

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:554: Resolver::ResolveDynamicAnyArgsAllowPrivate(klass,
internal_getter_name));
On 2013/09/27 00:21:01, siva wrote:
> const Function& function

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:562: 
On 2013/09/27 00:21:01, siva wrote:
> Add a comment here that InvokeDynamicFunction invokes NoSuchMethod if function
> is Null

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:579: Instance& result = Instance::Handle();
On 2013/09/27 00:21:01, siva wrote:
> This declaration can be moved inside the if (...) as it is only used there
> 
> const Instance& result = Result::Handle(InvokeLibrary......)

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:678: InvokeLibraryGetter(library, lookup_name, false));
On 2013/09/27 00:21:01, siva wrote:
> const Instance& result

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:682: Function& func = Function::Handle(
On 2013/09/27 00:21:01, siva wrote:
> const Function

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:690: Class& cls = Class::Handle(
On 2013/09/27 00:21:01, siva wrote:
> const Class

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1225: } else if (parts_len == 3) {
On 2013/09/27 00:21:01, siva wrote:
> this can just be:
> 
> else {
>   ASSERT(parts_len == 3);
>   ....
> }

Done.

https://codereview.chromium.org/23633002/diff/38001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1240: // We return a tuple (list) where the first slot
indicates whether we found a
On 2013/09/27 00:21:01, siva wrote:
> first slot is a boolean that indicates

Done.