[M] Change in dart/sdk[main]: [dart2bytecode] Visit new variables in Visitors of dart2bytecode
Chloe Stefantsova (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alexander Markov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
2 comments
_handleFunctionParameter(node);What would be the right way to declare variables uniformly in the new world, without duplicated code in visitVariableInitialization/visitPositionalParameter/visitNamedParameter? Maybe we should have visitScope or visitContext which would declare all variables in it?
void _allocateVariable(ExpressionVariable variable, {int? paramSlotIndex}) {The amount of `VariableDeclaration` -> `ExpressionVariable` and `.variable` -> `.expressionVariable` changes makes me wonder if we should:
1) Reserve a shorter class name `Variable` for expression variables, as it would make code much more readable. Current Variable can be called something like VariableBase, BaseVariable, AbstractVariable etc.
2) Perform global and mechanical renaming VariableDeclaration -> Variable and land it. Make their APIs compatible (e.g. Variable needs to have a getter and setter `initializer` even if it is going to delegate to VariableInitialization in the new world).
3) Mostly keep existing API even for new classes, just changing return values of some getters, e.g. VariableGet.variable would return Variable instead of VariableDeclaration, without changing the getter to `Variableget.expressionVariable`.
I think this would make overall migration much less painful - we would need a bit of preparation to make APIs compatible in a separate CL, then huge mechanical renaming CL and then we can switch to the new API under the flag without many repeated changes. WDYT?
Related details
- Chloe Stefantsova
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova voted and added 3 comments![]( "Open in Gerrit")
Votes added by Chloe Stefantsova
| Commit-Queue | +2 |
3 comments
Thank you for the review and the suggestions, Alex.
_handleFunctionParameter(node);What would be the right way to declare variables uniformly in the new world, without duplicated code in visitVariableInitialization/visitPositionalParameter/visitNamedParameter? Maybe we should have visitScope or visitContext which would declare all variables in it?
I was planning to do that during Phase 2, where the bytecode generator starts recognizing variable contexts. Then visiting the parameters themselves would not be necessary.
void _allocateVariable(ExpressionVariable variable, {int? paramSlotIndex}) {The amount of `VariableDeclaration` -> `ExpressionVariable` and `.variable` -> `.expressionVariable` changes makes me wonder if we should:
1) Reserve a shorter class name `Variable` for expression variables, as it would make code much more readable. Current Variable can be called something like VariableBase, BaseVariable, AbstractVariable etc.
2) Perform global and mechanical renaming VariableDeclaration -> Variable and land it. Make their APIs compatible (e.g. Variable needs to have a getter and setter `initializer` even if it is going to delegate to VariableInitialization in the new world).
3) Mostly keep existing API even for new classes, just changing return values of some getters, e.g. VariableGet.variable would return Variable instead of VariableDeclaration, without changing the getter to `Variableget.expressionVariable`.
I think this would make overall migration much less painful - we would need a bit of preparation to make APIs compatible in a separate CL, then huge mechanical renaming CL and then we can switch to the new API under the flag without many repeated changes. WDYT?
Thank you for the suggestions, Alex! I think some of them can be implemented right away, and some will need a further discussion.
1. I don't mind the renaming. I'm not extremely good at naming things, and I always welcome suggestions. I'll create a CL for that shortly.
2. A global and mechanical renaming can break on the boundary of migrated and unmigrated code. I can give it a go and see if it works well. In general though, doing the renaming as needed (lazily instead of eagerly) doesn't take too much time either. And I agree about the compatibility of the new and the old interfaces, and I try to follow that approach whenever possible. The delegating `initializer` setter, for example, was there from the very beginning in the `LocalVariable`.
3. Making `VariableGet.variable` return `Variable` instead of `VariableDeclaration` will be breaking for the backends that don't support the new variable model yet. That's the point of the `expressionVariable` getter, to use it in the code that supports the new model, while allowing the old `variable` getter to be used in the unmigrated code.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[dart2bytecode] Visit new variables in Visitors of dart2bytecode
Part of https://github.com/dart-lang/sdk/issues/61572
- M pkg/dart2bytecode/lib/bytecode_generator.dart
- M pkg/dart2bytecode/lib/local_vars.dart
- M pkg/kernel/lib/src/ast/variables.dart
Code-Review: +1 by Alexander Markov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
void _allocateVariable(ExpressionVariable variable, {int? paramSlotIndex}) {Chloe StefantsovaThe amount of `VariableDeclaration` -> `ExpressionVariable` and `.variable` -> `.expressionVariable` changes makes me wonder if we should:
1) Reserve a shorter class name `Variable` for expression variables, as it would make code much more readable. Current Variable can be called something like VariableBase, BaseVariable, AbstractVariable etc.
2) Perform global and mechanical renaming VariableDeclaration -> Variable and land it. Make their APIs compatible (e.g. Variable needs to have a getter and setter `initializer` even if it is going to delegate to VariableInitialization in the new world).
3) Mostly keep existing API even for new classes, just changing return values of some getters, e.g. VariableGet.variable would return Variable instead of VariableDeclaration, without changing the getter to `Variableget.expressionVariable`.
I think this would make overall migration much less painful - we would need a bit of preparation to make APIs compatible in a separate CL, then huge mechanical renaming CL and then we can switch to the new API under the flag without many repeated changes. WDYT?
Thank you for the suggestions, Alex! I think some of them can be implemented right away, and some will need a further discussion.
1. I don't mind the renaming. I'm not extremely good at naming things, and I always welcome suggestions. I'll create a CL for that shortly.
2. A global and mechanical renaming can break on the boundary of migrated and unmigrated code. I can give it a go and see if it works well. In general though, doing the renaming as needed (lazily instead of eagerly) doesn't take too much time either. And I agree about the compatibility of the new and the old interfaces, and I try to follow that approach whenever possible. The delegating `initializer` setter, for example, was there from the very beginning in the `LocalVariable`.
3. Making `VariableGet.variable` return `Variable` instead of `VariableDeclaration` will be breaking for the backends that don't support the new variable model yet. That's the point of the `expressionVariable` getter, to use it in the code that supports the new model, while allowing the old `variable` getter to be used in the unmigrated code.
Re: 3. What I mean is to use one class `Variable` both as a new `ExpressionVariable` and old `VariableDeclaration`. In such a case we would not need extra getter`VariableGet.expressionVariable`. AST would just always (regardless of the flag) use node Variable, either in place of an old VariableDeclaration or when new ExpressionVariable was used.
The prerequisite is that full VariableDeclaration API should be present in Variable, front-end should always generate Variable nodes (but maybe with different parents depending on the flag). Just think of Variable as a renaming of old VariableDecalaration and adding new API / implemented interfaces to fit into the new hierarchy.
I think this would result in a much smoother transition. We can prune Variable API after switching the flag everywhere.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |