Register synthesized mixin application classes in the library and reuse them

Register synthesized mixin application classes in the library and reuse them
when possible (fix issue 14318).

R=asiva@google.com, hausner@google.com, rmacnak@google.com

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

[regis, 2013-10-25 02:37:39]

This is a substantial refactoring of mixin handling in the VM. Synthesized mixin
application classes are not allocated by the parser anymore, but only later at
class finalization time. This allows them to be registered in their respective
libraries (the snapshot reader could not find them, issue 14318) and to be
shared (e.g. reused).

Unfortunately, this change broke mirrors. I am pretty sure this is due to the
mixin type anchored in the mixin application class, which is now the generic
version instead of the instantiated one, since the class may now be shared
between different instantiations (e.g. M<T>, instead of M<int>). The interface
is also generic now.

It is getting late, so I will work with Ryan tomorrow to figure out the issue
with mirrors. But feel free to start having a look.

Thanks,
Regis

[siva, 2013-10-25 16:55:53]

lgtm

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1364: // only interface to refer to the respective
type parameters of the mixin
I am finding it hard to parse this comment.

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1414: // TODO(regis): handle type bounds.
maybe open an issue and use the issue number here in the TODO instead of ldap
handles.

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:2293: mixin_app_class_name =
Symbols::New(mixin_app_class_name);
I am wondering if it makes sense to only make it a symbol in the case when
mixin_app_class.IsNull()

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:2301: generic_mixin_type =
Type::New(Class::Handle(mixin_type.type_class()),
maybe have a mixin_type_class handle outside the loop and assign here as
mixin_type_class = mixin_type.type_class();
generic....

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/object.h#newcode4292
runtime/vm/object.h:4292: // The class finalizer creates synthesized classes S&M
and S&M&N if then do not
if they

[hausner, 2013-10-25 17:11:48]

LGTM with comments.

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1364: // only interface to refer to the respective
type parameters of the mixin
what are "only" interfaces?

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1414: // TODO(regis): handle type bounds.
Thank you :-)

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1555: void ClassFinalizer::ApplyMixinTypedef(const
Class& mixin_app_class) {
Unrelated to this change, but maybe it's time to rename this function, since we
no longer have mixin typedefs in the syntax.

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:12241: return
AbstractType::Handle(MixinTypeAt(0)).token_pos();
Should this be the position of the super type instead of the first mixin? Before
this change, MixinTypeAt(0) was the super type.

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/object.h#newcode4292
runtime/vm/object.h:4292: // The class finalizer creates synthesized classes S&M
and S&M&N if then do not
then->they

[regis, 2013-10-25 18:34:25]

Thanks all.

Ryan, thanks for mirror patch. All tests pass now. Please have a look.

-- Regis

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1364: // only interface to refer to the respective
type parameters of the mixin
On 2013/10/25 16:55:53, siva wrote:
> I am finding it hard to parse this comment.

Clarified and added an example.

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1364: // only interface to refer to the respective
type parameters of the mixin
On 2013/10/25 17:11:49, hausner wrote:
> what are "only" interfaces?

Bad wording. The mixin application class has a unique interface identical to the
mixin type. I rewrote the comment.

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1414: // TODO(regis): handle type bounds.
On 2013/10/25 16:55:53, siva wrote:
> maybe open an issue and use the issue number here in the TODO instead of ldap
> handles.

Issue 14453 filed.

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1414: // TODO(regis): handle type bounds.
On 2013/10/25 17:11:49, hausner wrote:
> Thank you :-)

Oh good, I was afraid you would be upset I grab it from you :-)

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:1555: void ClassFinalizer::ApplyMixinTypedef(const
Class& mixin_app_class) {
On 2013/10/25 17:11:49, hausner wrote:
> Unrelated to this change, but maybe it's time to rename this function, since
we
> no longer have mixin typedefs in the syntax.

Right. We also have a is_mixin_typedef() function.
Added TODO, but I still need a better name. Using "mixin application" is too
vague, since we use that term for synthesized classes. Maybe "mixin alias"?

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:2293: mixin_app_class_name =
Symbols::New(mixin_app_class_name);
On 2013/10/25 16:55:53, siva wrote:
> I am wondering if it makes sense to only make it a symbol in the case when
> mixin_app_class.IsNull()

Good point. I moved the line inside the if branch.
I did the same for the typedef class below.

https://codereview.chromium.org/42723003/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:2301: generic_mixin_type =
Type::New(Class::Handle(mixin_type.type_class()),
On 2013/10/25 16:55:53, siva wrote:
> maybe have a mixin_type_class handle outside the loop and assign here as
> mixin_type_class = mixin_type.type_class();
> generic....

Done.

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:12241: return
AbstractType::Handle(MixinTypeAt(0)).token_pos();
On 2013/10/25 17:11:49, hausner wrote:
> Should this be the position of the super type instead of the first mixin?
Before
> this change, MixinTypeAt(0) was the super type.

I think the position of the first mixin type is better. It used to be the
position of class MixinAppAt(0), which was already the position of the fist
mixin type. The super type was MixinAppAt(0)->super_type().

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

https://codereview.chromium.org/42723003/diff/1/runtime/vm/object.h#newcode4292
runtime/vm/object.h:4292: // The class finalizer creates synthesized classes S&M
and S&M&N if then do not
On 2013/10/25 16:55:53, siva wrote:
> if they

Done.

https://codereview.chromium.org/42723003/diff/1/runtime/vm/object.h#newcode4292
runtime/vm/object.h:4292: // The class finalizer creates synthesized classes S&M
and S&M&N if then do not
On 2013/10/25 16:55:53, siva wrote:
> if they

Done.

[rmacnak, 2013-10-25 18:40:27]

LGTM

[regis, 2013-10-25 19:46:38]

Committed patchset #3 manually as r29276 (presubmit successful).