Skip to content

Improve error reporting for 'this()' initializer from 'record struct' constructor#58430

Merged
cston merged 1 commit intodotnet:mainfrom
cston:58328-2
Jan 3, 2022
Merged

Improve error reporting for 'this()' initializer from 'record struct' constructor#58430
cston merged 1 commit intodotnet:mainfrom
cston:58328-2

Conversation

@cston
Copy link
Copy Markdown
Contributor

@cston cston commented Dec 20, 2021

Follow-up for:
Report error if 'record struct' constructor calls default parameterless constructor #58339

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1), assuming CI is passing. Consider using more descriptive commit title when merging.

@cston cston changed the title Updates to PR #58339 Improve error reporting for 'this()' initializer from 'record struct' constructor Dec 20, 2021
@cston cston requested review from a team and removed request for a team December 22, 2021 00:28
@cston
Copy link
Copy Markdown
Contributor Author

cston commented Jan 3, 2022

@dotnet/roslyn-compiler, please review, thanks.

{
var constructorSymbol = (MethodSymbol)this.ContainingMember();
if (!constructorSymbol.IsStatic &&
if (isInstanceConstructor(out MethodSymbol constructorSymbol) &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: merge into outer if now that we can?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's address separately if necessary.

Copy link
Copy Markdown
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 a nit to consider

@jcouv jcouv self-assigned this Jan 3, 2022
@jcouv jcouv added the Feature - Records Records label Jan 3, 2022
@cston cston merged commit 9bd26d8 into dotnet:main Jan 3, 2022
@ghost ghost added this to the Next milestone Jan 3, 2022
@cston cston deleted the 58328-2 branch January 3, 2022 17:37
@Cosifne Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022
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