Fix resolution and canonicalization of typedefs and function types in

Fix resolution and canonicalization of typedefs and function types in
preparation of the new typedef syntax.

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/291ed0c5452cc2cc6302ea17987e2e0963571d1c

[regis, 2016-12-23 02:37:46]

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

[siva, 2016-12-27 18:08:28]

LGTM with some comments.

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/class_finali...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/class_finali...
runtime/vm/class_finalizer.cc:568: // we cannot reset the token position to
TokenPosition::kNoSource.
Is it necessary to drop the other fields? I can understand dropping the parent
function but do the others matter?

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

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2102: #endif
Why is this done only under the DEBUG flag?

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:12030: // we cannot reset the token position to
TokenPosition::kNoSource.
Ditto comment about dropping non parent_function fields.

[regis, 2016-12-27 18:28:48]

Thanks!

I'll work on a separate cl making the tree shaker more robust before committing
this cl.

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/class_finali...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/class_finali...
runtime/vm/class_finalizer.cc:568: // we cannot reset the token position to
TokenPosition::kNoSource.
On 2016/12/27 18:08:28, siva wrote:
> Is it necessary to drop the other fields? I can understand dropping the parent
> function but do the others matter?

No, it is not necessary to drop them, but they are not meaningful anymore after
canonicalization, since an arbitrary signature function may be picked to
represent others at different token positions and from different owners. Mirrors
still rely on reparsing the source of typedefs obtained via owner and token
position. Canonical representents are not shared for typedefs.

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

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2102: #endif
On 2016/12/27 18:08:28, siva wrote:
> Why is this done only under the DEBUG flag?

This variable is not used anymore and the assignment is preventative. Sure, I
can always do it it the compiler does not complain.

https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:12030: // we cannot reset the token position to
TokenPosition::kNoSource.
On 2016/12/27 18:08:28, siva wrote:
> Ditto comment about dropping non parent_function fields.

I'll add a comment explaining why the owner and token position are meaningless
after canonicalization.

[regis, 2016-12-28 00:48:14]

On 2016/12/27 18:28:48, regis wrote:
> Thanks!
> 
> I'll work on a separate cl making the tree shaker more robust before
committing
> this cl.
> 
>
https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/class_finali...
> File runtime/vm/class_finalizer.cc (right):
> 
>
https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/class_finali...
> runtime/vm/class_finalizer.cc:568: // we cannot reset the token position to
> TokenPosition::kNoSource.
> On 2016/12/27 18:08:28, siva wrote:
> > Is it necessary to drop the other fields? I can understand dropping the
parent
> > function but do the others matter?
> 
> No, it is not necessary to drop them, but they are not meaningful anymore
after
> canonicalization, since an arbitrary signature function may be picked to
> represent others at different token positions and from different owners.
Mirrors
> still rely on reparsing the source of typedefs obtained via owner and token
> position. Canonical representents are not shared for typedefs.
> 
> https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/parser.cc#ne...
> runtime/vm/parser.cc:2102: #endif
> On 2016/12/27 18:08:28, siva wrote:
> > Why is this done only under the DEBUG flag?
> 
> This variable is not used anymore and the assignment is preventative. Sure, I
> can always do it it the compiler does not complain.
> 
>
https://codereview.chromium.org/2592263004/diff/20001/runtime/vm/parser.cc#ne...
> runtime/vm/parser.cc:12030: // we cannot reset the token position to
> TokenPosition::kNoSource.
> On 2016/12/27 18:08:28, siva wrote:
> > Ditto comment about dropping non parent_function fields.
> 
> I'll add a comment explaining why the owner and token position are meaningless
> after canonicalization.

I have addressed your comments. While adding more comments, I realized that I
could reset these fields for non-typedef signature functions. I also addressed
the todos I had just added. I'll commit shortly. If you have more comments, I
will address them in a subsequent cl. Thanks!

[regis, 2016-12-28 00:55:26]

Description was changed from

==========
Fix resolution and canonicalization of typedefs and function types in
preparation of the new typedef syntax.
==========

to

==========
Fix resolution and canonicalization of typedefs and function types in
preparation of the new typedef syntax.

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/291ed0c5452cc2cc6302ea17987e2e0963571d1c
==========

[regis, 2016-12-28 00:55:28]

Committed patchset #3 (id:40001) manually as
291ed0c5452cc2cc6302ea17987e2e0963571d1c (presubmit successful).