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

VB init-only: Conversions in init-only phase #50770

Merged
merged 4 commits into from
Jan 28, 2021

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Jan 26, 2021

This is a VB "counterpart" of #50424.

@AlekseyTs AlekseyTs marked this pull request as ready for review January 26, 2021 15:02
@AlekseyTs AlekseyTs requested a review from a team as a code owner January 26, 2021 15:02
@AlekseyTs AlekseyTs requested a review from jcouv January 26, 2021 17:41
@AlekseyTs
Copy link
Contributor Author

@jcouv, @dotnet/roslyn-compiler Please review.

333fred
333fred previously approved these changes Jan 26, 2021
jcouv
jcouv previously approved these changes Jan 26, 2021
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 2)

@jcouv jcouv self-assigned this Jan 26, 2021
@jaredpar
Copy link
Member

@AlekseyTs can we re-target this to release/dev16.9?

@AlekseyTs AlekseyTs changed the base branch from master to release/dev16.9 January 27, 2021 01:35
@AlekseyTs
Copy link
Contributor Author

can we re-target this to release/dev16.9?

Done

@AlekseyTs
Copy link
Contributor Author

@333fred, @jcouv Please review Additional changes made to reflect LDM decision.

@AlekseyTs AlekseyTs dismissed stale reviews from jcouv and 333fred January 28, 2021 00:02

New changes

@AlekseyTs
Copy link
Contributor Author

@jcouv Could you please take another look?

1 similar comment
@AlekseyTs
Copy link
Contributor Author

@jcouv Could you please take another look?

@AlekseyTs
Copy link
Contributor Author

@jaredpar Do we need to get any special sign-off to merge into release/dev16.9?

Select Case receiverOpt.Kind
Case BoundKind.MeReference, BoundKind.MyBaseReference, BoundKind.MyClassReference
Return True
Case BoundKind.Parenthesized
Copy link
Member

@jcouv jcouv Jan 28, 2021

Choose a reason for hiding this comment

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

Looks like this fixes the handling for parenthesized receiver in With. Do we have a test for that case? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this fixes the handling for parenthesized receiver in With. Do we have a test for that case?

EvaluationInitOnlySetter_10


In reply to: 566277477 [](ancestors = 566277477)

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 4) with a test question (nit)

@AlekseyTs
Copy link
Contributor Author

@jaredpar Good to merge?

@AlekseyTs AlekseyTs merged commit 9d9b8fb into dotnet:release/dev16.9 Jan 28, 2021
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