forked from ShadowTheAge/yafc
-
Notifications
You must be signed in to change notification settings - Fork 21
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
.NET 8 code cleanup #114
Merged
Merged
.NET 8 code cleanup #114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
At the time of check, IDE0047 was still active for some reason even when suppressed. Therefore, I had to change the related settings to 'none' so it is suppressed for good. Now, it should be completely up to a developer whether to use parentheses or not.
All tricky vars should've been replaced in the previous code cleanup, but it seems some of them have been missed. Let's fix that.
Also use explicit type for clarity.
String.Contains(char) offers better performance than String.Contains(string).
The usage of GeneratedRegexAttribute allows to generate a regex implementation at compile time.
This PR usually ran code analysis on each enabled inspection because running code cleanup on the whole solution takes around 15 minutes. However, I ran code cleanup once to check if the state of the project is what I expected. Most of what the cleanup did is converting var to nint.
The char version is faster than the string one. Also, enable inspections that fixed nothing. It seems that some errors are only visible if you open the file and let analyzer do its thing. Running analysis on the whole solution no longer showed the suggestion that was there before. Full names of the enabled inspections: CA1806: Do not ignore method results CA1840: Use 'Environment.CurrentManagedThreadId' CA1859: Use concrete types when possible for improved performance CA1861: Avoid constant arrays as arguments CA2249: Consider using 'string.Contains' instead of 'string.IndexOf' IDE0070: Use 'System.HashCode' IDE0270: Use coalesce expression The char version is faster than the string one.
…mportAttribute' Full inspection name: SYSLIB1054: Use 'LibraryImportAttribute' instead of 'DllImportAttribute' to generate P/Invoke marshalling code at compile time From my understanding, this change should quicken the load time slightly because we generate more resulting code at compile time. There is a problem that I could not initialize System.Type[] as a variable in UnmanagedCallConv to reduce the amount of boilerplate code -- the argument needs to be either a constant or what it is right now. Feel free to make it more concise if you can. Also, this commit enables CA1061: Do not hide base class methods that did not add any fixes. Testing done on an existing save: built, launched, briefly checked basic functionality.
Also enable other inspections that didn't fix anything: IDE0008: Use explicit type IDE1006: Naming Styles CA1816: Dispose methods should call SuppressFinalize CA1866: Use char overload CA1869: Cache and reuse 'JsonSerializerOptions' instances
…od(string)' for string with single char This inspection was enabled before, but detected the violation only now.
Replace Array.Empty<>() with [] Also, enable other inspections that didn't fix anything: IDE0051: Remove unused private members IDE0060: Remove unused parameter IDE0078: Use pattern matching IDE0083: Use pattern matching IDE0251: Make member 'readonly'
Making a member static reduces the size of the created objects for that class. Also enable several other inspections that didn't fix anything: CA1834: Consider using 'StringBuilder.Append(char)' when applicable CA2211: Non-constant fields should not be visible, IDE0028: Simplify collection initialization IDE0290: Use primary constructor IDE0305: Simplify collection initialization
Also improve the explanations of several suppressions.
Dorus
approved these changes
May 7, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
nice 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The migration to .NET 8 allowed some of the inspections to suggest more improvements. These improvements are applied in this PR.
The method was the same as the previous one. I muted all inspections that were suggesting edits and then unmuted them one by one.
QA: loaded the usual save file and checked basic functionality.