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

Preserve changes that are still relevant after revert of #66311 #66440

Merged
merged 2 commits into from
Jan 21, 2023

Conversation

AlekseyTs
Copy link
Contributor

Related to #63221.

@AlekseyTs AlekseyTs requested review from 333fred and jcouv January 17, 2023 15:42
@AlekseyTs AlekseyTs requested a review from a team as a code owner January 17, 2023 15:43
@AlekseyTs
Copy link
Contributor Author

@333fred, @jcouv, @dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Contributor Author

@jcouv, @dotnet/roslyn-compiler For the second review

_factory.Sequence(new BoundExpression[] { _factory.AssignmentExpression(cache, intermediateRef) }, cache),
receiverType,
isRef: true),
#pragma warning disable format
Copy link
Member

@jcouv jcouv Jan 19, 2023

Choose a reason for hiding this comment

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

nit: What's the formatting issue? Should we file a tracking issue? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the formatting issue? Should we file a tracking issue?

IDE wants the initializer on a separate line. I think it is by design, but I do not like the design. I have no intention to fight for its change though.

private IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker(BoundLoweredConditionalAccess conditionalAccess)
: base()
{
_conditionalAccess = conditionalAccess;
Copy link
Member

@jcouv jcouv Jan 19, 2023

Choose a reason for hiding this comment

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

nit: it looks like the only part we need to hold onto is the Id #WontFix

private bool? _result;

private IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker(BoundLoweredConditionalAccess conditionalAccess)
: base()
Copy link
Member

@jcouv jcouv Jan 19, 2023

Choose a reason for hiding this comment

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

nit: is the base initializer needed? #Closed

@@ -564,6 +569,60 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces
}
}

private sealed class IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
Copy link
Member

@jcouv jcouv Jan 19, 2023

Choose a reason for hiding this comment

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

IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker

nit: a comment would be nice "we must use a temp when ..." #Closed

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1) with some nits to consider

@jcouv jcouv self-assigned this Jan 19, 2023
@AlekseyTs AlekseyTs enabled auto-merge (squash) January 20, 2023 19:46
@AlekseyTs AlekseyTs merged commit 4d0af36 into dotnet:main Jan 21, 2023
@ghost ghost added this to the Next milestone Jan 21, 2023
@Cosifne Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 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.

4 participants