Handle creating ParameterMirrors for the parameters of forwarding constructors.

Handle creating ParameterMirrors for the parameters of forwarding constructors.

BUG=http://dartbug.com/17823
R=hausner@google.com, regis@google.com

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

[rmacnak, 2014-03-27 01:01:29]

https://codereview.chromium.org/212883009/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/212883009/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:3189: return mixin() != Type::null() ||
is_mixin_app_alias();
I think the old condition became invalid during the changes to support generics
in mixin app aliases.

https://codereview.chromium.org/212883009/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/212883009/diff/1/runtime/vm/object.h#newcode1580
runtime/vm/object.h:1580: // Target information for forwarding constructors of
mixin applications.
"Target" isn't quite right, but I can't think of a better name.

[rmacnak, 2014-03-27 01:36:07]

https://codereview.chromium.org/212883009/diff/40001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/212883009/diff/40001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:1711: while
(super_class.IsAnonymousMixinApplication()) {
Do we have tests where the superclass is another mixin app alias?

[hausner, 2014-03-27 17:10:27]

LGTM but please also get the LGTM from Regis. There may be mixin class
subtleties I don't understand.

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

https://codereview.chromium.org/212883009/diff/60001/runtime/lib/mirrors.cc#n...
runtime/lib/mirrors.cc:132: if (func.IsForwardingConstructor()) {
Curious: why don't you do this test at the beginning of the function?

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:5097: return Function::Cast(obj).raw();
Asserting that obj is indeed a function?

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:9031: if (!current_class().IsAnonymousMixinApplication()) {
I think this is wrong. The scoping is the same whether the mixin is anonymous or
named. You can add a test if you like.

OTOH, maybe I don't understand what the difference is between a mixin
application class and an anoymous mixin application class. Regis can comment if
I'm wrong.

[regis, 2014-03-27 18:23:58]

See my comments. I would not proceed with this rename.

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:3189: return mixin() != Type::null() ||
is_mixin_app_alias();
See my comment in parser.cc first.

I would not make this change. Things are getting very confusing if you consider
an alias to be a mixin application class.

We need to distance ourselves from the terminology of the spec.

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:3194: return IsMixinApplication() && !is_mixin_app_alias();
This is what you need to fix. IsAnonymousMixinApplication() is identical to
IsMixinApplication(). I think this was not your intent when you introduced it.

I think you can achieve what you need using only IsMixinApplication() and
is_mixin_app_alias().

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:5097: return Function::Cast(obj).raw();
On 2014/03/27 17:10:27, hausner wrote:
> Asserting that obj is indeed a function?

I am pretty sure that the Cast checks the actual type in debug mode.

On a different note, I find it slightly hacky to stuff the function object into
the data_ field. You could use a RedirectionTarget object here as well. I
understand that you only need one field and this is wasteful to allocate a
RedirectionTarget object. Are you sure you do not need any other field, like a
type or an identifier? :-) That's fine you can leave it as is.

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/212883009/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:9031: if (!current_class().IsAnonymousMixinApplication()) {
On 2014/03/27 17:10:27, hausner wrote:
> I think this is wrong. The scoping is the same whether the mixin is anonymous
or
> named. You can add a test if you like.
> 
> OTOH, maybe I don't understand what the difference is between a mixin
> application class and an anoymous mixin application class. Regis can comment
if
> I'm wrong.

I think the change is correct. If the current class is a mixin application
alias, we should use it to resolve locals, such as its type parameters.

The confusion comes from the fact that this cl is changing names to match the
spec. However, the new names do not match the comments in the code anymore. This
is bad.

Before the change we had:
mixin application
mixin application alias

With this change, they become:
anonymous mixin application
mixin application

But the comments still refer to the previous terms.

I am not sure if it is worth using the terms of the spec, which does not know
anything about the implementation. I'd rather stick with mixin application and
mixin application alias, which makes IsAnonymousMixinApplication a bad name and
mostly reverts this cl.

See how I would fix this in object.cc

[rmacnak, 2014-03-27 22:14:25]

Reverted to the current behavior of IsMixinApplication.  Removed
IsAnonymousMixinApplication. Documented spec/implementation disagreement about
the name. Removed setting the ultimate target function for forwarding
constructors. This won't work when restrictions on mixin constructors are
lifted. Instead skipping the attempt to reparse a parameter list when the method
is synthetic and saying parameters of synthetic methods are always final with no
metadata.

[regis, 2014-03-27 22:26:45]

LGTM!

[rmacnak, 2014-03-28 01:22:09]

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