ParameterMirror: Implement some missing getters.

ParameterMirror: Implement some missing getters.

Fixes issue 11334 and issue 11684 and issue 11567.

R=asiva@google.com

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

[Michael Lippautz (Google), 2013-08-01 22:00:14]

This is as far as we can get without reparsing or storing additional information
on the function (that's where parameter infos are stored).

We have a generic test for parameter mirror which still fails, because we are
missing further properties (for instance hasDefaultValue).

[rmacnak, 2013-08-02 00:50:19]

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

https://codereview.chromium.org/21624006/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:330: GET_NON_NULL_NATIVE_ARGUMENT(Instance, owner,
arguments->NativeArgAt(1));
Flip the order of the arguments and make this an instance method.

https://codereview.chromium.org/21624006/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1157: GET_NON_NULL_NATIVE_ARGUMENT(Instance, owner,
arguments->NativeArgAt(1));
As above.

[Michael Lippautz (Google), 2013-08-02 17:05:44]

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

https://codereview.chromium.org/21624006/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:330: GET_NON_NULL_NATIVE_ARGUMENT(Instance, owner,
arguments->NativeArgAt(1));
On 2013/08/02 00:50:19, Ryan Macnak wrote:
> Flip the order of the arguments and make this an instance method.

Done.

https://codereview.chromium.org/21624006/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1157: GET_NON_NULL_NATIVE_ARGUMENT(Instance, owner,
arguments->NativeArgAt(1));
On 2013/08/02 00:50:19, Ryan Macnak wrote:
> As above.

Done.

[Michael Lippautz (Google), 2013-08-06 22:13:58]

+Peter

Peter: We already have a test for these changes (lib/mirrors/parameter_test),
but we still fail it because of another getter that didn't implement yet.
dart2js lies about the getter (hasDefaultValue -> always false). Suggestions?

[siva, 2013-08-06 23:54:54]

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

https://codereview.chromium.org/21624006/diff/6001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:57: func.NumOptionalNamedParameters();
I would probably write this code as follows to make it more readable:

const intptr_t implicit_param_count = func.NumImplicitParameters();
const intptr_t non_implicit_param_count = func.NumParameters() -
implicit_param_count;
const intptr_t index_of_first_optional_param = non_implicit_param_count -
func.NumOptionalParameters();
const intptr_t index_of_first_named_param = non_implicit_param_count -
func.NumOptionalNamedParameters();

https://codereview.chromium.org/21624006/diff/6001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:66: args.SetAt(1,
String::Handle(func.ParameterNameAt(implicit_cnt + i)));
String::Handle() should be hoisted out of the loop.

[Michael Lippautz (Google), 2013-08-07 00:21:42]

Thanks for making this more readable!

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

https://codereview.chromium.org/21624006/diff/6001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:57: func.NumOptionalNamedParameters();
On 2013/08/06 23:54:54, siva wrote:
> I would probably write this code as follows to make it more readable:
> 
> const intptr_t implicit_param_count = func.NumImplicitParameters();
> const intptr_t non_implicit_param_count = func.NumParameters() -
> implicit_param_count;
> const intptr_t index_of_first_optional_param = non_implicit_param_count -
> func.NumOptionalParameters();
> const intptr_t index_of_first_named_param = non_implicit_param_count -
> func.NumOptionalNamedParameters();
> 

Done.

https://codereview.chromium.org/21624006/diff/6001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:66: args.SetAt(1,
String::Handle(func.ParameterNameAt(implicit_cnt + i)));
On 2013/08/06 23:54:54, siva wrote:
> String::Handle() should be hoisted out of the loop.

Done.

[siva, 2013-08-07 00:36:31]

lgtm

[ahe, 2013-08-12 18:19:55]

Suggestion for a test:

Create a class with methods that you want to reflect on.

Reflect on these methods to obtain a method mirror, say 'method'.

Import stringify.dart.

expect('blah blah blah', method.parameters.toList());

Extend stringifyParameter as needed.

[Michael Lippautz (Google), 2013-08-12 19:34:20]

On 2013/08/12 18:19:55, ahe wrote:
> Suggestion for a test:
> 
> Create a class with methods that you want to reflect on.
> 
> Reflect on these methods to obtain a method mirror, say 'method'.
> 
> Import stringify.dart.
> 
> expect('blah blah blah', method.parameters.toList());
> 
> Extend stringifyParameter as needed.

We already have a test that fits pretty well: lib/mirrors/parameter_test

I removed the check for ParameterMirror.hasDefaultValue from the
stringifyParameter function, as the VM doesn't support it yet (throwing an
exception), and dart2js returns false in all cases (even with default values),
essentially lying about it. 

We can add the check later again.

[ahe, 2013-08-13 21:32:55]

Test looks good!

https://codereview.chromium.org/21624006/diff/26006/tests/lib/mirrors/stringi...
File tests/lib/mirrors/stringify.dart (right):

https://codereview.chromium.org/21624006/diff/26006/tests/lib/mirrors/stringi...
tests/lib/mirrors/stringify.dart:70: //                   the VM and dart2js.
Remove extra whitespace at beginning of line.

[Michael Lippautz (Google), 2013-08-13 21:37:09]

https://codereview.chromium.org/21624006/diff/26006/tests/lib/mirrors/stringi...
File tests/lib/mirrors/stringify.dart (right):

https://codereview.chromium.org/21624006/diff/26006/tests/lib/mirrors/stringi...
tests/lib/mirrors/stringify.dart:70: //                   the VM and dart2js.
On 2013/08/13 21:32:55, ahe wrote:
> Remove extra whitespace at beginning of line.

Done.

[Michael Lippautz (Google), 2013-08-13 21:38:41]

Committed patchset #7 manually as r26086 (presubmit successful).