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

order modifiers #744

Merged
merged 10 commits into from
Jun 10, 2023
Merged

order modifiers #744

merged 10 commits into from
Jun 10, 2023

Conversation

glmnet
Copy link
Contributor

@glmnet glmnet commented Oct 19, 2022

an attempt to fix #725

I believe now comes the "opinionated" part, I could implement this order default value (which I found after this quick implementation):

public, private, protected, internal, file, static, extern, new, virtual, abstract, sealed, override, readonly, unsafe, required, volatile, async

from:

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0036

Copy link
Collaborator

@shocklateboy92 shocklateboy92 left a comment

Choose a reason for hiding this comment

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

Hey, I took a quick look at the test files and yeah I'm not a super big fan of putting async before public.

Would very much prefer if you sorted them in the order you listed (as per style rule IDE00036).

@belav
Copy link
Owner

belav commented Oct 24, 2022

The other part of this is figuring out how to get SyntaxNodeComparer to not complain that modifiers were reordered. A lot of that code is generated by CSharpier.FakeGenerators. Maybe it needs to call a new method CompareModifiers instead of the existing CompareLists, unless CompareLists can determine that is is comparing modifiers.

@belav belav mentioned this pull request Oct 29, 2022
@voxlz
Copy link

voxlz commented Jun 2, 2023

Is this forgotten? I just ran into this issue, and would love to see it implemented.

shocklateboy92
shocklateboy92 previously approved these changes Jun 6, 2023

public int Compare(string? x, string? y)
{
int GetIndex(string? value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this a nested method?
I feel like it could be a private static method in that class.

{
int GetIndex(string? value)
{
var result = Array.IndexOf(DefaultOrdered, value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this gets called for every comparison of every declaration in the source code, right?
It might be worth optimizing to be constant time access by say, initializing a static dictionary of string to index.

Src/CSharpier/SyntaxPrinter/Modifiers.cs Outdated Show resolved Hide resolved
#if !SILVERLIGHT
unsafe
#endif
static String Method() { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Damn, how did you even come up with this case 😲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sort modifiers
4 participants