Don't capture BoundNode when creating a LazyNoneOperation#36055
Conversation
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Approve
Submit feedback approving these changes.
|
|
||
| internal sealed class CSharpLazyNoneOperation : LazyNoneOperation | ||
| { | ||
| private readonly Func<BoundNode, ImmutableArray<IOperation>> _getChildren; |
There was a problem hiding this comment.
This is still capturing. We should instead hold onto the CSharpOperationFactory here, and call GetIOperationChildren off that instance below. #Closed
| Friend NotInheritable Class VisualBasicLazyNoneOperation | ||
| Inherits LazyNoneOperation | ||
|
|
||
| Private ReadOnly _getChildren As Func(Of BoundNode, ImmutableArray(Of IOperation)) |
333fred
left a comment
There was a problem hiding this comment.
To actually address the bug, I'd like to totally remove the lambda allocation. Because it's still capturing the CSharpOperationFactory, the lambda isn't cached. We should just use the same pattern as the other nodes use and hold onto the CSharpOperationFactory manually. You'll have to change the accessibility of the GetIOperationChildren method, but we've already taken that pattern through the rest of the code and the factory/nodes are pretty strongly coupled already.
|
@333fred |
|
Then what we can do is have |
…for LazyNoneOperation
|
Done. |
| //TODO: Implement UsingLocalDeclaration operations correctly. | ||
| // For now we return an implicit operationNone, with a single child consisting of the using declaration parsed as if it were a standard variable declaration | ||
| // For now we return an implicit operationNone, | ||
| // and GetIoperationChildren will return a single child |
There was a problem hiding this comment.
o [](start = 28, length = 1)
Nit: spelling
There was a problem hiding this comment.
There was a problem hiding this comment.
fixed, and fixed, assuming I got the right spelling mistake :-)
333fred
left a comment
There was a problem hiding this comment.
LGTM other than the minor spelling and trailing whitespace issue. @dotnet/roslyn-compiler for a second review.
|
Thanks @YairHalberstadt! |
Fixes #34918