Reflection on generics.

Reflection on generics.

BUG=
R=ahe@google.com, regis@google.com

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

[gbracha, 2013-08-05 21:55:31]

Tests look good with comments

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
File tests/lib/mirrors/generics_test.dart (right):

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:13: class C extends A<num, int> {}  // Same
as class C extends A<dynamic>.
Comment is wrong.

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:65: typeParameters(reflect(new
B<num>()).type, ['']);
Why do you sometimes use a list with an empty string and sometimes an empty
list? Empty list is cleaner.
Ditto two lines below.

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:99:
libraryMirrors.classes.values.forEach((classMirror) {
Isn't it libraryMirror rather than libraryMirrors?

[ahe, 2013-08-06 16:45:43]

Test LGTM!

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
File tests/lib/mirrors/generics_test.dart (right):

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:13: class C extends A<num, int> {}  // Same
as class C extends A<dynamic>.
On 2013/08/05 21:55:31, gbracha wrote:
> Comment is wrong.

Why?

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:19: typeParameters(mirror, parameterNames){
Space before {

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:24: typeArguments(mirror, argumentMirrors){
Ditto.

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:89: Expect.equals(reflectClass(A),
reflect(new A<num>()).type.originalDeclaration);
Long lines.

https://codereview.chromium.org/22292002/diff/6001/tests/lib/mirrors/generics...
tests/lib/mirrors/generics_test.dart:90: Expect.equals(reflectClass(B),
reflect(new B     ()).type.originalDeclaration);
Please don't use ASCII art, that is, remove the extra spaces.

[rmacnak, 2013-08-07 20:17:11]

Added implementation

[siva, 2013-08-07 23:43:43]

Adding Regis to the review list.

[regis, 2013-08-08 17:06:34]

LGTM with a few comments.

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

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:103: const AbstractType& type,
Parameter 'type' added but not used?
If it is for future work, add a TODO. Or wait before adding the extra parameter.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:177: type_parameters.IsNull() || type_parameters.Length()
== 0
We like parenthesis (a lot!).
In any case, this would be simpler:
const Bool& is_generic = (cls.NumTypeParameters() == 0) ? ...
And you can spare the type_parameters handle.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:191: static RawAbstractType* RawTypeOfClass(const Class&
klass) {
Why 'klass' and 'cls' elsewhere? I think 'cls' is preferred.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:192: AbstractTypeArguments& type_arguments =
AbstractTypeArguments::Handle();
You can save a handle and pass directly Object::null_abstract_type_arguments().

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:581: }
You can replace the above 5 lines with
const intptr_t num_params = klass.NumTypeParameters();

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

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:517: _computeTypeArguments(_reflectedType));
Are we OK with such long lines? Just asking, not a Dart programmer myself.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:777: _TypedefMirror_referent(_reflectee), null
/*BOGUS?*/);
BOGUS? Can you be more specific?

[rmacnak, 2013-08-08 20:04:06]

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

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:103: const AbstractType& type,
On 2013/08/08 17:06:34, regis wrote:
> Parameter 'type' added but not used?
> If it is for future work, add a TODO. Or wait before adding the extra
parameter.

Leftover from intermediate implementation that completely switched the reflectee
to a Type. Would only work if there was a distinct Type for the declaration, but
we decided against this. Removed.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:177: type_parameters.IsNull() || type_parameters.Length()
== 0
On 2013/08/08 17:06:34, regis wrote:
> We like parenthesis (a lot!).
> In any case, this would be simpler:
> const Bool& is_generic = (cls.NumTypeParameters() == 0) ? ...
> And you can spare the type_parameters handle.
> 
Done.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:191: static RawAbstractType* RawTypeOfClass(const Class&
klass) {
On 2013/08/08 17:06:34, regis wrote:
> Why 'klass' and 'cls' elsewhere? I think 'cls' is preferred.

Different authors. Switched to cls, as its use is in the majority.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:192: AbstractTypeArguments& type_arguments =
AbstractTypeArguments::Handle();
On 2013/08/08 17:06:34, regis wrote:
> You can save a handle and pass directly
Object::null_abstract_type_arguments().

Done.

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:581: }
On 2013/08/08 17:06:34, regis wrote:
> You can replace the above 5 lines with
> const intptr_t num_params = klass.NumTypeParameters();

Done.

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

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:517: _computeTypeArguments(_reflectedType));
On 2013/08/08 17:06:34, regis wrote:
> Are we OK with such long lines? Just asking, not a Dart programmer myself.

The style-guide only discourages lines over 80 columns, not forbids them. It
seemed preferable to splitting the constructor name. Folks really shouldn't be
editing Dart in a terminal window anyway...

https://codereview.chromium.org/22292002/diff/43001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:777: _TypedefMirror_referent(_reflectee), null
/*BOGUS?*/);
On 2013/08/08 17:06:34, regis wrote:
> BOGUS? Can you be more specific?

Replaced with TODO(12282): Deal with generic typedefs.

[rmacnak, 2013-08-08 20:07:03]

Committed patchset #4 manually as r25942 (presubmit successful).