Implement the invoke methods (invoke, getField, setField, newInstance, apply) as internal natives.

Implement the invoke methods (invoke, getField, setField, newInstance, apply) as internal natives.

R=asiva@google.com

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

[rmacnak, 2013-07-09 21:04:53]

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

https://codereview.chromium.org/18463003/diff/5002/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1399: // object.h/.c is questionable.
Questionable

https://codereview.chromium.org/18463003/diff/5002/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1466: static RawObject* ResolveConstructor(const char*
current_func,
This is the same as the embedding API's version. I decided not to put it in
Resolver because all the error messages are out of place there. Eventually, we
will probably move to having reflective invocations produce the same errors as
their non-reflective counterparts, eg all the errors here would be
NoSuchMethodErrors. When we get there, it will probably make sense to move this
function to Resolver.

https://codereview.chromium.org/18463003/diff/5002/runtime/vm/dart_entry.h
File runtime/vm/dart_entry.h (right):

https://codereview.chromium.org/18463003/diff/5002/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:148: static RawObject* InvokeConstructor(const Class&
klass,
We do not yet have a variant supporting named/optional arguments, but the mirror
invocations don't support named/optional arguments in general yet.

[rmacnak, 2013-07-11 21:27:56]

Handle attempting a setter on null.

Ensure all exceptions are catchable. The wrapped exception is still an APIError
as before, later they will become NoSuchMethodErrors.

Setup expected failures of new tests for dart2js.

[siva, 2013-07-11 22:53:37]

Here is my initial comment list, some of these might have already been addressed
by you in your new patch set I willl go through that now.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1395: }
This seems kind of hacky, I realize it is a temporary solution but why not have
a version of CreateInstanceMirror which works with Dart_Handles using the API
and implement the plain VM code here.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1405: }
We already have a bunch of IsXXXClassId(index) methods in raw_object.h

You could add IsApiErrorClassId and IsLanguageErrorClassId and
use them instead of IsApiError also morph IsCompilationError (see comment
below).

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1433:
String::Handle(String::New("MirroredUncaughtExceptionError")),
You probably want to add MirroredUncaughtExceptionError to the symbols list and
use Symbols::MirroredUncaughtExceptionError(), here

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1445: }
Once you have all VM code there would be no ApiError right? We should probably
abstract this into the IsCompilationError() function which will during the
transition time also check for ApiErrors.
Similarly you could have a getMessage() static method that hides the Api stuff.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1453:
String::Handle(String::New("MirroredCompilationError")),
Ditto comment about adding MirroredCompilationError as a symbol.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1457: const Instance& stack = Instance::Handle();
shadowing local variables like this is not a good practice.
You could probably just do a:
stack = Stack::null() here and have the same effect.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1536: 
extra blank line?

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1562: Exceptions::PropagateError(error);
Why are we creating an ApiError and propagating it here? Wouldn't this just
unwind all the way to the invocation frame.

I would think you want to throw a NoSuchMethod exception here..

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1598: Exceptions::PropagateError(error);
Ditto question about ApiError

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1628: // Check for real fields and user-defined setters
missing '.' at end of comment.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/vm/dart_ent...
File runtime/vm/dart_entry.cc (right):

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/vm/dart_ent...
runtime/vm/dart_entry.cc:159: ultimate_klass ^= klass.raw();
why not
Class& ultimate_klass = Class::Handle(klass.raw());

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/vm/dart_ent...
runtime/vm/dart_entry.cc:162: ultimate_constructor ^= constructor.raw();
Ditto.

[siva, 2013-07-11 23:50:59]

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1312: static RawError* CreateMirroredError(const Error&
error) {
Why can't this just be
void ThrowMirroredError(const Error& error);

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1341: return UnhandledException::New(mirrored_exc,
stack);
Here you would call:
Exceptions::ThrowByType(kMirroredUncaughtExceptionError, args);

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1362: return UnhandledException::New(mirrored_exc,
stack);
Here you would call
Exceptions::ThrowByType(kMirroredCompilationError, args);

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1439: DEFINE_NATIVE_ENTRY(InstanceMirror_invoke, 4) {
Argument 0 is not used at all in these methods, should these just be static
methods (here and all the native methods below).

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1467: Object& result = Object::Handle();
Can you share the 'arg' handle above and the 'result' handle here
by just naming it 'obj'.

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1484: UNREACHABLE();
ThrowMirroredError(Error::Cast(result));

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1569: UNREACHABLE();
Instead of this elaborate scheme of first creating an ApiError and then morphing
it into an unhandled exception using CreateMirroredError why don't you add a
type kMirroredCompilationError to exceptions.h
and use
Exceptions::ThrowByType(kMirroredCompilationError, args);
here.

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1607: DEFINE_NATIVE_ENTRY(ClassMirror_invoke, 4) {
Argument 0 is not used at all here, maybe this should just be a static native
methods then?

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1637: Exceptions::PropagateError(error);
Instead of this elaborate scheme of first creating an ApiError and then morphing
it into an unhandled exception using CreateMirroredError why don't you add a
type kMirroredCompilationError to exceptions.h
and use
Exceptions::ThrowByType(kMirroredCompilationError, args);
here.

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors_impl.dart:608: native 'ClassMirror_invokeSetter';
The implementation of these native methods does not use the 'this' parameter at
all, why can't these just be static methods?

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors_impl.dart:868: native 'LibraryMirror_invokeSetter';
Ditto question about static methods.

[rmacnak, 2013-07-11 23:59:23]

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1395: }
On 2013/07/11 22:53:37, siva wrote:
> This seems kind of hacky, I realize it is a temporary solution but why not
have
> a version of CreateInstanceMirror which works with Dart_Handles using the API
> and implement the plain VM code here.

The intention is that there will be no plain VM code version. Because
InstanceMirror's reflectees are ordinary Dart objects, they can be created in
Dart code without calling to natives.

The need to create an InstanceMirror within a native only arises for
MirroredUnhandledExceptionMirrors. We intend to get rid of the wrapped
exceptions, but I'd rather do it in a separate CL.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1405: }
On 2013/07/11 22:53:37, siva wrote:
> We already have a bunch of IsXXXClassId(index) methods in raw_object.h
> 
> You could add IsApiErrorClassId and IsLanguageErrorClassId and
> use them instead of IsApiError also morph IsCompilationError (see comment
> below).

Now I see that CompilationError is an alias for LanguageError, for which there
is a proper IsLanguageError on Object.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1457: const Instance& stack = Instance::Handle();
On 2013/07/11 22:53:37, siva wrote:
> shadowing local variables like this is not a good practice.
> You could probably just do a:
> stack = Stack::null() here and have the same effect.

It's not shadowing. They are in "sibling" scopes.

https://chromiumcodereview.appspot.com/18463003/diff/5002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:1562: Exceptions::PropagateError(error);
On 2013/07/11 22:53:37, siva wrote:
> Why are we creating an ApiError and propagating it here? Wouldn't this just
> unwind all the way to the invocation frame.
> 
> I would think you want to throw a NoSuchMethod exception here..

Right. These ApiErrors (wrapped in MirroredCompilationErrors) should become
NoSuchMethod exceptions. This is a breaking change, which belongs in a separate
CL as mentioned above.

[rmacnak, 2013-07-12 00:04:09]

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1312: static RawError* CreateMirroredError(const Error&
error) {
On 2013/07/11 23:50:59, siva wrote:
> Why can't this just be
> void ThrowMirroredError(const Error& error);

Good point.

https://chromiumcodereview.appspot.com/18463003/diff/5302669702856704/runtime...
runtime/lib/mirrors.cc:1439: DEFINE_NATIVE_ENTRY(InstanceMirror_invoke, 4) {
On 2013/07/11 23:50:59, siva wrote:
> Argument 0 is not used at all in these methods, should these just be static
> methods (here and all the native methods below).

Argument 0 is the mirror. These natives are polymorphic w/r/t ObjectMirror.

[rmacnak, 2013-07-12 16:49:21]

Now there are no uses of ApiError in the new natives.

[rmacnak, 2013-07-12 20:12:46]

Added the internal native for ClosureMirror_apply, which follows the same
pattern as the other invokes. This is the last type of invoke, so many of the
old implementation functions can now be removed.

[siva, 2013-07-15 05:42:57]

This CL is becoming too big. Can we not add more new functionality into this CL.
Let us land this CL with the current functionality.

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

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1158: exc_string ^= Instance::null();
exc_string is already null why set it to Null again?

could be just:
if (!exc_string_or_error.IsError()) {
  exc_string ^ exc_string_or_error.raw();
}

Also shouldn't exc_string be a String

String& exc_string = String::Handle();

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1276: const Instance& receiver =
Instance::CheckedHandle(arguments->NativeArgAt(1));
In the dart file you call this parameter reflectee, maybe it would be more
readable if you called it reflectee here too (same comment probably also applies
to other functions here where you use receiver).

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1336: // members.
How come there isn't a similar check for a null receiver here (similar to the
code you have for the setter).

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1340: while (!klass.IsNull()) {
LookupInstanceField also?

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1350: // Invoke the getter or DNU and return the result.
DNU is not a standard abbreviation we use

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1361: args_descriptor);
indentation.....

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1391: // null's class doesn't handle lookups gracefully,
fall through to the DNU
Ditto comment about DNU

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1437: return result.raw();
This pattern of InvokeNoSuchMethod or InvokeFunction etc. seems to be repeated
in each of the functions, maybe you could abstract this into a static function
and call it everywhere making the code concise.

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1586: // Invoke the getter and return the result.
Invoke the setter....

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1632: String& klass_name = String::Handle(klass.Name());
const String& klass_name = String::Handle(.....);

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1634: String::Handle(String::Concat(klass_name,
Symbols::Dot()));
if (!constructor_name.IsNull()) {

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1638: Function& constructor =
const Function& constructor =

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1645: Object& result =
const Object& result = ....

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1678: function =
library.LookupFunctionAllowPrivate(function_name);
const Function& function = Function::Handle(.....);

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1705: positional_args));
const Object& result = .....

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/vm/dart_en...
File runtime/vm/dart_entry.cc (right):

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/vm/dart_en...
runtime/vm/dart_entry.cc:159: ultimate_klass ^= klass.raw();
incoming parameter is already a Class so why not just use
Class& ultimate_klass = Class::Handle(klass.raw());

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/vm/dart_en...
runtime/vm/dart_entry.cc:162: ultimate_constructor ^= constructor.raw();
Ditto comment about constructor being already a Function.

[rmacnak, 2013-07-15 19:53:22]

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

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1158: exc_string ^= Instance::null();
On 2013/07/15 05:42:58, siva wrote:
> exc_string is already null why set it to Null again?
> 
> could be just:
> if (!exc_string_or_error.IsError()) {
>   exc_string ^ exc_string_or_error.raw();
> }
> 
> Also shouldn't exc_string be a String
> 
> String& exc_string = String::Handle();

There were more cases when this code dealt with unwind errors. I missed
simplifying this when they were removed.

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1276: const Instance& receiver =
Instance::CheckedHandle(arguments->NativeArgAt(1));
On 2013/07/15 05:42:58, siva wrote:
> In the dart file you call this parameter reflectee, maybe it would be more
> readable if you called it reflectee here too (same comment probably also
applies
> to other functions here where you use receiver).

Hm, calling it reflectee is more uniform, but there is a non-uniformity in that
InstanceMirrors' reflectees are unwrapped. Every kind of mirror has a different
kind of reflectee anyway, so it is probably better to call this reflectee as you
suggest.

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1336: // members.
On 2013/07/15 05:42:58, siva wrote:
> How come there isn't a similar check for a null receiver here (similar to the
> code you have for the setter).

Null's class failed for field lookups, not function lookups, which we only need
to do for setters. On closer inspection, I see that Class::LookupField is
missing a null check that Class::LookupFunction[AllowPrivate] has, so I am
adding the missing check there and removing the special case from
InstanceMirror_invokeSetter.

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1340: while (!klass.IsNull()) {
On 2013/07/15 05:42:58, siva wrote:
> LookupInstanceField also?

No. InstanceMirror_invokeGetter is the ideal case: uniform access for fields and
user-defined getters. With regard to setters, final fields do not introduce an
implicit setter. Without an explicit check we might find an inherited setter,
but we wish to report an error. As I understand it, the language will change so
final fields introduce implicit setters that throw an exception. When this
happens, the mirror code for invokeSetter can be as simple as invokeGetter.

Classes and libraries are more complicated because they don't introduce implicit
getters or setters.

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1350: // Invoke the getter or DNU and return the result.
On 2013/07/15 05:42:58, siva wrote:
> DNU is not a standard abbreviation we use

Smalltalkism, switched to noSuchMethod

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1437: return result.raw();
On 2013/07/15 05:42:58, siva wrote:
> This pattern of InvokeNoSuchMethod or InvokeFunction etc. seems to be repeated
> in each of the functions, maybe you could abstract this into a static function
> and call it everywhere making the code concise.

Extracted as ReflectivelyInvokeDynamicFunction. A corresponding
ReflectivelyInvokeStaticFunction for classes/libraries will make sense when the
lookup errors become NoSuchMethodErrors in the next CL.

https://chromiumcodereview.appspot.com/18463003/diff/70001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1634: String::Handle(String::Concat(klass_name,
Symbols::Dot()));
On 2013/07/15 05:42:58, siva wrote:
> if (!constructor_name.IsNull()) {

Attempting to pass null fails before the native, in string/symbol conversion.
Adding the check anyway to be defensive.

[siva, 2013-07-15 20:52:52]

lgtm

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

https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1271: // exceptions. Wrap and propgrate any compilation
errors.
propagate.

https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1718: Field& field =
Field::Handle(library.LookupFieldAllowPrivate(getter_name));
const Field& field

https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1774: Field& field =
Field::Handle(library.LookupFieldAllowPrivate(setter_name));
const Field& field

https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
runtime/lib/mirrors_impl.dart:186: final _reflectee; // May be a MirrorReference
or an ordinary object
missing '.'

https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/vm/dart_en...
File runtime/vm/dart_entry.cc (right):

https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/vm/dart_en...
runtime/vm/dart_entry.cc:195: Object& result =
const Object& result

https://chromiumcodereview.appspot.com/18463003/diff/86001/tests/lib/mirrors/...
File tests/lib/mirrors/mirrors_test.dart (right):

https://chromiumcodereview.appspot.com/18463003/diff/86001/tests/lib/mirrors/...
tests/lib/mirrors/mirrors_test.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
I think we don't change the original year for just updates to the file.

[rmacnak, 2013-07-15 21:14:30]

On 2013/07/15 20:52:52, siva wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
> File runtime/lib/mirrors.cc (right):
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
> runtime/lib/mirrors.cc:1271: // exceptions. Wrap and propgrate any compilation
> errors.
> propagate.
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
> runtime/lib/mirrors.cc:1718: Field& field =
> Field::Handle(library.LookupFieldAllowPrivate(getter_name));
> const Field& field
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
> runtime/lib/mirrors.cc:1774: Field& field =
> Field::Handle(library.LookupFieldAllowPrivate(setter_name));
> const Field& field
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
> File runtime/lib/mirrors_impl.dart (right):
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/lib/mirror...
> runtime/lib/mirrors_impl.dart:186: final _reflectee; // May be a
MirrorReference
> or an ordinary object
> missing '.'
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/vm/dart_en...
> File runtime/vm/dart_entry.cc (right):
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/runtime/vm/dart_en...
> runtime/vm/dart_entry.cc:195: Object& result =
> const Object& result
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/tests/lib/mirrors/...
> File tests/lib/mirrors/mirrors_test.dart (right):
> 
>
https://chromiumcodereview.appspot.com/18463003/diff/86001/tests/lib/mirrors/...
> tests/lib/mirrors/mirrors_test.dart:1: // Copyright (c) 2013, the Dart project
> authors.  Please see the AUTHORS file
> I think we don't change the original year for just updates to the file.

Got these.

[rmacnak, 2013-07-15 21:14:49]

Committed patchset #14 manually as r25024 (presubmit successful).

[Florian Schneider, 2013-07-16 09:39:56]

dbc:

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

https://chromiumcodereview.appspot.com/18463003/diff/99001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1321: args.SetAt((i + num_receiver), arg);
Please don't over-parenthesize. I don't think there is doubt about the
precedence of the plus-operator and the comma-separator in any scenario.

https://chromiumcodereview.appspot.com/18463003/diff/99001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1328: (number_of_arguments + 1),
Please don't over-parenthesize. I don't think there is doubt about the
precedence of the plus-operator and the comma-separator in any scenario.

https://chromiumcodereview.appspot.com/18463003/diff/99001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1329: Resolver::kIsQualified));
This is an enum where the function expects an int. This is fixed by one of my
CLs already, but be careful when copying code around.

[ahe, 2013-07-16 10:40:46]

DBC

I'd really appreciate if you could CC me on future CLs that add new mirror
tests.

https://codereview.chromium.org/18463003/diff/99001/tests/lib/mirrors/mirrors...
File tests/lib/mirrors/mirrors_test.dart (right):

https://codereview.chromium.org/18463003/diff/99001/tests/lib/mirrors/mirrors...
tests/lib/mirrors/mirrors_test.dart:40: expect(instMirror.invoke(const
Symbol("notDefined"), []).reflectee,
It would be great if you could avoid making further modifications to this test.

Instead, prefer writing small tests that test a specific part of the mirror API,
much like the tests I have added to this directory.

Big tests tend to take longer to run.  Eventually, a large test will test so
much that it starts timing out.  Also, if a part of a big test is broken, you
lose all test coverage when you mark it as failing.  So in general, we prefer
small tests.

Also, you should avoid using the unittest framework in new tests.  It is likely
that the unittest framework will start relying more and more on reflection. 
This means we will have a hard time testing the mirror API.

Also, when you modified this test, something unexpected happened: it started
passing in dart2js' CSP mode.  I have no idea why that is, but clearly there was
something in the old version that caused problems.  That is another reason to
avoid modifying existing tests.