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

Avoid deep recursion while emitting deeply nested logical conditions #66993

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

AlekseyTs
Copy link
Contributor

Fixes #66900.

@AlekseyTs AlekseyTs requested a review from a team as a code owner February 22, 2023 14:50
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

2 similar comments
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

@lewing
Copy link
Member

lewing commented Feb 24, 2023

cc @jaredpar

// fallThrough:

object fallThrough = null;
var stack = ArrayBuilder<(BoundExpression? condition, StrongBox<object?> destBox, bool sense)>.GetInstance();
Copy link
Member

@jjonescz jjonescz Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is dest wrapped in a StrongBox? Why not store just object? in the stack? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is dest wrapped in a StrongBox? Why not store just object? in the stack?

Its value is mutable and is supposed to be shared between different items on the stack (including mutations). If we simply store object?, we will store its current value (often null) and won't be able to observe the changes. We need an indirection in order to be able share the mutations as well. StrongBox gives us an indirection.

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review

@AlekseyTs
Copy link
Contributor Author

@cston , @jjonescz Please review a small adjustment in commit 2

@AlekseyTs AlekseyTs merged commit 3b9dbde into dotnet:main Feb 24, 2023
@ghost ghost added this to the Next milestone Feb 24, 2023
@RikkiGibson RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flow into dotnet/runtime main blocked
5 participants