Serialize typedef parameters (including function typed ones) to Kernel and use it to resynthesize t…

Serialize typedef parameters (including function typed ones) to Kernel and use it to resynthesize typedefs from Kernel.

R=ahe@google.com, brianwilkerson@google.com, sigmund@google.com
BUG=

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

[Brian Wilkerson, 2017-07-26 21:29:50]

lgtm, at least for the analyzer portions

[Siggi Cherem (dart-lang), 2017-07-26 22:32:44]

https://codereview.chromium.org/2990783002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart
(right):

https://codereview.chromium.org/2990783002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart:83:
KernelFunctionTypeParameters buildParameters(LibraryBuilder library) {
Any reason to have this code here and not directly in function_type_alias
builder? This seems to be more about the funciton-type-alias (typedef) and not
about the function-type itself.

https://codereview.chromium.org/2990783002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart:90: if
(formal.isRequired) parameters.requiredParameterCount++;
I have a slight preference to do things like in the function further above: use
locals for the list/int, and at the end create the instance with the values
passed in. Then KernelFunctionTypeParameters.requiredParameterCount can be
final.

If you end up moving this to function-type-alias-builder, you may be able to
delete the helper KernelFunctionTypeParameters class and set the parameters
directly instead.

https://codereview.chromium.org/2990783002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart:94: }
should we sort named parameters as well?

[scheglov, 2017-07-27 16:40:50]

PTAL

https://codereview.chromium.org/2990783002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart
(right):

https://codereview.chromium.org/2990783002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart:83:
KernelFunctionTypeParameters buildParameters(LibraryBuilder library) {
On 2017/07/26 22:32:44, Siggi Cherem (dart-lang) wrote:
> Any reason to have this code here and not directly in function_type_alias
> builder? This seems to be more about the funciton-type-alias (typedef) and not
> about the function-type itself.

I moved to KernelFunctionTypeAliasBuilder and removed
KernelFunctionTypeParameters.

[Brian Wilkerson, 2017-07-27 16:51:58]

still lgtm

[Siggi Cherem (dart-lang), 2017-07-27 18:31:09]

thanks, lgtm after dealing with the sorting question.

https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:127:
}
Just one pending question: do we need to sort by name? or do we normally keep
the declaration order in other places?

[scheglov, 2017-07-27 18:33:53]

https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:127:
}
On 2017/07/27 18:31:09, Siggi Cherem (dart-lang) wrote:
> Just one pending question: do we need to sort by name? or do we normally keep
> the declaration order in other places?

I will add sorting, but I don't know why we need this.

[scheglov, 2017-07-27 18:43:46]

https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:127:
}
On 2017/07/27 18:33:53, scheglov wrote:
> On 2017/07/27 18:31:09, Siggi Cherem (dart-lang) wrote:
> > Just one pending question: do we need to sort by name? or do we normally
keep
> > the declaration order in other places?
> 
> I will add sorting, but I don't know why we need this.

Actually, I see that we sort named parameters in types, but don't see sorting
for AST.
So, I will keep not add sorting.
This might be helpful to keep original order for the time when we start matching
Analyzer AST with element model.

[scheglov, 2017-07-27 18:53:20]

Description was changed from

==========
Serialize typedef parameters (including function typed ones) to Kernel and use
it to resynthesize typedefs from Kernel.

R=ahe@google.com, brianwilkerson@google.com, sigmund@google.com
BUG=
==========

to

==========
Serialize typedef parameters (including function typed ones) to Kernel and use
it to resynthesize typedefs from Kernel.

R=ahe@google.com, brianwilkerson@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-07-27 18:53:27]

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

[Siggi Cherem (dart-lang), 2017-07-27 19:10:44]

On 2017/07/27 18:43:46, scheglov wrote:
>
https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
> File
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
> (right):
> 
>
https://codereview.chromium.org/2990783002/diff/20001/pkg/front_end/lib/src/f...
>
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:127:
> }
> On 2017/07/27 18:33:53, scheglov wrote:
> > On 2017/07/27 18:31:09, Siggi Cherem (dart-lang) wrote:
> > > Just one pending question: do we need to sort by name? or do we normally
> keep
> > > the declaration order in other places?
> > 
> > I will add sorting, but I don't know why we need this.
> 
> Actually, I see that we sort named parameters in types, but don't see sorting
> for AST.

Sounds good - this is why I was asking (because I saw it on the types). Thanks
for checking about the ASTs

> So, I will keep not add sorting.
> This might be helpful to keep original order for the time when we start
matching
> Analyzer AST with element model.