Introduce a SignatureData class so that signature functions can store both their

Introduce a SignatureData class so that signature functions can store both their
parent function and their signature type. This is required to implement the
new function type syntax.

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/889b0039525d20102f4ba76413b311d6c15aff68

[regis, 2016-12-20 23:07:49]

regis@google.com changed reviewers:
+ asiva@google.com

[regis, 2016-12-20 23:55:03]

regis@google.com changed reviewers:
+ rmacnak@google.com

[regis, 2016-12-20 23:55:03]

Ryan, would you mind having a look at the TODO when you are back? Not urgent.
Thanks.

[siva, 2016-12-21 01:00:55]

lgtm

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

https://codereview.chromium.org/2591823002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7259: if (value.IsResolved()) {
Instead of an if check can we assert here that is it resolved?

ASSERT(value.IsResolved());

https://codereview.chromium.org/2591823002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7275: return "SignatureData class";
Do you think it would be more beneficial to expand this to

return OS::SCreate(Thread::Current()->zone(), "signature_type:%s,
parent_function:%s", signature_type...., parent_function);

[regis, 2016-12-21 01:56:21]

Thanks!

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

https://codereview.chromium.org/2591823002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7259: if (value.IsResolved()) {
On 2016/12/21 01:00:55, siva wrote:
> Instead of an if check can we assert here that is it resolved?
> 
> ASSERT(value.IsResolved());

No, that would not work, because we may set a still unresolved type, and in this
case, we have to keep the parent function until resolution. That is the point of
this cl.

It is well possible that it would work today, but not anymore as soon as the new
typedef syntax lands.

https://codereview.chromium.org/2591823002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7275: return "SignatureData class";
On 2016/12/21 01:00:55, siva wrote:
> Do you think it would be more beneficial to expand this to
> 
> return OS::SCreate(Thread::Current()->zone(), "signature_type:%s,
> parent_function:%s", signature_type...., parent_function);

Good point. I added more info, here and in ClosureData::ToCString() and
RedirectionData::ToCString().

[regis, 2016-12-21 02:01:51]

Description was changed from

==========
Introduce a SignatureData class so that signature functions can store both their
parent function and their signature type. This is required to implement the
new function type syntax.
==========

to

==========
Introduce a SignatureData class so that signature functions can store both their
parent function and their signature type. This is required to implement the
new function type syntax.

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/889b0039525d20102f4ba76413b311d6c15aff68
==========

[regis, 2016-12-21 02:01:52]

Committed patchset #4 (id:60001) manually as
889b0039525d20102f4ba76413b311d6c15aff68 (presubmit successful).

[kustermann, 2016-12-21 16:23:41]

kustermann@google.com changed reviewers:
+ kustermann@google.com

[kustermann, 2016-12-21 16:23:42]

@Regis: I think this CL caused all our precompilation-release builders to fail.

I've spent a bit of time looking into it. It looks like when we write out a
precompiled snapshot via clustered_snapshot.cc we try to serialize a class which
has been invalidated (i.e. it's Class::id() is kInvalidCid, tree shaken away).

Strangely enough this issue doesn't seem to happen in debug mode.

Looking a bit more, it seems that in debug mode we do the following which seems
to make it work:

runtime/vm/precompiler.cc:
2046 #if defined(DEBUG)
2047     class_table->Unregister(cid);
2048 #endif

Does this sound reasonable?

Ryan,Florian probably know why this is only done in debug mode.