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

Create RecordDeclarationOrganizer #48101

Merged
merged 11 commits into from
Jan 5, 2021
Merged

Conversation

Youssef1313
Copy link
Member

No description provided.

@Youssef1313 Youssef1313 requested a review from a team as a code owner September 27, 2020 22:58
@Youssef1313 Youssef1313 reopened this Sep 27, 2020
@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Sep 28, 2020
@Youssef1313 Youssef1313 changed the title Classify RecordDeclaration's OuterOrdering as Types Create RecordDeclarationOrganizer Sep 28, 2020
Comment on lines 48 to +60
var initial =
@"class C {
$@"{typeKind} C {{
int C;
int B;
int A;
}";
}}";

var final =
@"class C {
$@"{typeKind} C {{
int A;
int B;
int C;
}";
}}";
Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi This looks like a nice feature that I was never aware it's existed. Can I ask where this feature is in Visual Studio? I can't find any option along the words "organize declarations"

@CyrusNajmabadi
Copy link
Member

@Youssef1313 tests are failing :)

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Fixed the failing test. Is this ready to merge?

MemberDeclarationsOrganizer.Organize(syntax.Members, cancellationToken),
syntax.CloseBraceToken,
syntax.SemicolonToken);
}
Copy link
Member

Choose a reason for hiding this comment

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

can you jsut do syntax.WithModifiers(...).WithMembers(...);?

Copy link
Member Author

@Youssef1313 Youssef1313 Oct 4, 2020

Choose a reason for hiding this comment

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

@CyrusNajmabadi Sounds reasonable. But I followed the same pattern used in other implementations in the same folder to be consistent with them. If I'd change it here, I'd change it everywhere in the same folder too. Do you want me to update all of them in this PR? or keep it for later PR?

Note: I think WithModifiers(...).WithMembers(...) will cause 2 allocations, while Update will cause only 1 allocation? (could that be the reason why other organizers are written in this way?)

I might be wrong, so correct me if so please.

I also would like to know where this feature exist in Visual Studio.

Copy link
Member

Choose a reason for hiding this comment

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

Two allocations is fine here :-)

Copy link
Member

Choose a reason for hiding this comment

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

I also would like to know where this feature exist in Visual Studio.

The feature never shipped. But it has come up recently as something we might like to resurrect

@CyrusNajmabadi CyrusNajmabadi merged commit 6f35aa5 into dotnet:master Jan 5, 2021
@ghost ghost added this to the Next milestone Jan 5, 2021
@Youssef1313 Youssef1313 deleted the patch-32 branch January 5, 2021 07:55
@Cosifne Cosifne modified the milestones: Next, 16.9 Jan 27, 2021
@JoeRobich JoeRobich modified the milestones: 16.9, 16.9.P4 Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants