Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate TypeArguments into separate TypeParameters (binding) and TypeArguments (use) classes. #43901

Closed
sstrickl opened this issue Oct 23, 2020 · 4 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@sstrickl
Copy link
Contributor

In CL 168101, we save the default arguments (i.e., results of instantiating to bounds) calculated by the CFE in TypeParameter objects. However, these default arguments are only needed when the instantiator of a TypeArguments object containing only TypeParameters has not provided any explicit type arguments, and won't be used when instantiating individual TypeParameters.

Instead, we should separate out binding and use occurrences of TypeArguments into two classes, TypeParameters and TypeArguments. The former will replace the use of TypeArguments to store the type parameters of classes and functionss, and it can store a vector that represents the entire vector instantiated to bounds without adding overhead to all TypeArguments objects. We can also move any methods on TypeArguments that only make sense for binding positions to the new TypeParameters class.

@sstrickl sstrickl added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Oct 23, 2020
@sstrickl sstrickl self-assigned this Oct 23, 2020
@sstrickl
Copy link
Contributor Author

/cc @mkustermann @crelier

@crelier
Copy link
Contributor

crelier commented Jan 15, 2021

@sstrickl Now that my CL introducing a proper FunctionType has landed, I wanted to tackle this refactoring, unless you feel strongly about doing it yourself, of course.
This would also resolve the proper canonicalization of function type literals, for which I recently committed a temporary workaround.

@sstrickl
Copy link
Contributor Author

No worries, I have other things to focus on at the moment, go for it! :)

(Yaaaaaaay, separating binding and use!)

@crelier
Copy link
Contributor

crelier commented Jan 19, 2021

Great! Thanks. I am on it.

dart-bot pushed a commit that referenced this issue May 5, 2021
…n the VM."

This reverts commit 8a21ab1.

Reason for revert: Test failure: http://b/187227619

Original change's description:
> [VM/runtime] Refactor the representation of type parameters in the VM.
>
> This introduces a new VM internal class 'TypeParameters' representing the declaration of a list of type parameters, either in a class or function.
> The reference to (or use of) a type parameter is still represented by the existing 'TypeParameter' class.
>
> Fixes #43901
> Fixes #45763
>
> TEST=existing ones and a regression test
>
> Change-Id: I1fde808bf753cc1cb829f2c4383c1836651cee80
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/189942
> Commit-Queue: Régis Crelier <regis@google.com>
> Reviewed-by: Alexander Markov <alexmarkov@google.com>

TBR=rmacnak@google.com,alexmarkov@google.com,regis@google.com,sstrickl@google.com

Change-Id: If12caa1a84cb6d1c1b8225589f3c994d25abb120
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198282
Reviewed-by: Michal Terepeta <michalt@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>
Commit-Queue: Michal Terepeta <michalt@google.com>
dart-bot pushed a commit that referenced this issue May 5, 2021
…n the VM."

This is a reland of 8a21ab1

Original change's description:
> [VM/runtime] Refactor the representation of type parameters in the VM.
>
> This introduces a new VM internal class 'TypeParameters' representing the declaration of a list of type parameters, either in a class or function.
> The reference to (or use of) a type parameter is still represented by the existing 'TypeParameter' class.
>
> Fixes #43901
> Fixes #45763
>
> TEST=existing ones and a regression test
>
> Change-Id: I1fde808bf753cc1cb829f2c4383c1836651cee80
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/189942
> Commit-Queue: Régis Crelier <regis@google.com>
> Reviewed-by: Alexander Markov <alexmarkov@google.com>

This fixes #45911

TEST=existing ones and a regression test

Change-Id: I709d38b1df3d73fe3c9796d5aca3cbbdcf77fd38
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198380
Commit-Queue: Régis Crelier <regis@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

2 participants