[L] Change in dart/sdk[main]: [cfe,vm] Add 'this' expression to super invocation nodes
Chloe Stefantsova (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alexander Markov
- Johnni Winther
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 5 comments![]( "Open in Gerrit")
writeNode(node.thisExpression);Consider serializing receiver before `node.name` to match other invocation nodes and keep evaluation order consistent (receiver before value/arguments).
(also for other nodes)
Expression thisExpression;Considering calling this field `receiver` to match other nodes.
(also in other nodes)
{required Expression thisExpression})Consider making this parameter the first positional parameter in order to match other nodes - receiver is usually after `kind` (if any) and before `name`.
(also in other nodes)
thisExpression: new ThisExpression(),Nit: we don't follow CFE's non-standard "always use `new`" style in pkg/vm, so `new` can be omitted.
SkipExpression(); // skip this_expression.We should serialize receiver before `value` and use it with
```
instructions += BuildExpression();
```
at L2691 instead of `LoadLocal(parsed_function()->receiver_var())`.
(also for other nodes)
Related details
- Chloe Stefantsova
- Johnni Winther
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Johnni Winther voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chloe Stefantsova
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova added 6 comments![]( "Open in Gerrit")
Thanks for taking a look and for the comments, Alex! I need some more of your guidance in updating `runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc`.
writeNode(node.thisExpression);Consider serializing receiver before `node.name` to match other invocation nodes and keep evaluation order consistent (receiver before value/arguments).
(also for other nodes)
Good suggestion! I started applying it, but encountered an issue with modifying `runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc`, which I describe in the comment on that file.
Considering calling this field `receiver` to match other nodes.
(also in other nodes)
Done
Consider making this parameter the first positional parameter in order to match other nodes - receiver is usually after `kind` (if any) and before `name`.
(also in other nodes)
Done
Nit: we don't follow CFE's non-standard "always use `new`" style in pkg/vm, so `new` can be omitted.
Done
SkipExpression(); // skip this_expression.We should serialize receiver before `value` and use it with
```
instructions += BuildExpression();
```
at L2691 instead of `LoadLocal(parsed_function()->receiver_var())`.(also for other nodes)
I made an attempt to update the code as you're suggesting, but I still can't make the VM to compile. I'd appreciate any help :) I've uploaded the state I ended up in in this CL.
Additionally, for that reading order to work, `receiver` should come before `value`, but after `name`, which contradicts the desire to put `receiver` before the `name` for uniformity with other invocation nodes.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| 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")
SkipExpression(); // skip this_expression.Chloe StefantsovaWe should serialize receiver before `value` and use it with
```
instructions += BuildExpression();
```
at L2691 instead of `LoadLocal(parsed_function()->receiver_var())`.(also for other nodes)
I made an attempt to update the code as you're suggesting, but I still can't make the VM to compile. I'd appreciate any help :) I've uploaded the state I ended up in in this CL.
Additionally, for that reading order to work, `receiver` should come before `value`, but after `name`, which contradicts the desire to put `receiver` before the `name` for uniformity with other invocation nodes.
It turned out to be a bit more involved, but I think I figured that out. I uploaded a patchset with the necessary changes to this CL. (We might need to find another reviewer for the VM changes as I cannot approve my own changes.)
Maybe having a `name` before `receiver` could simplify VM's code a bit, but I think we should do this consistently on all invocation nodes if we decide it is worth doing (and separately from this CL).
Related details
- Chloe Stefantsova
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 2 comments![]( "Open in Gerrit")
I also added missing traversal of receiver nodes in visitors and setting parent pointers. There are still some front-end test failures, but all other bots are green.
writeNode(node.thisExpression);Consider serializing receiver before `node.name` to match other invocation nodes and keep evaluation order consistent (receiver before value/arguments).
Chloe Stefantsova(also for other nodes)
Alexander MarkovGood suggestion! I started applying it, but encountered an issue with modifying `runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc`, which I describe in the comment on that file.
Done
Related details
- Chloe Stefantsova
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |