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

Minor performance and style improvements #20020

Merged
merged 12 commits into from
May 18, 2022
Merged

Minor performance and style improvements #20020

merged 12 commits into from
May 18, 2022

Conversation

eduherminio
Copy link
Contributor

Feel free to cherry-pick only some of the commits (or none of them!)

All these changes have been detected by static analysis and auto-resolved using VS suggestions, otherwise I wouldn't date to send such a length PR.

  • Count() -> .Count() or .Length when possible
  • Any() -> .Count() or .Length when possible
  • Remove setters when possible
  • Use pattern matching when possible (avoiding double casting)
  • Simplify string comparison (x.ToLowerInvariant() == y -> string.Equals(x, y, StringComparison.InvariantCultureIgnoreCase))
  • Simplify collection initialization when possible
  • Protected constructor in abstract class
  • Remove unnecessarily interpolated strings
  • Simplify always true exceptions (b805481, please review carefully)

Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Changes/Ideas look good to me overall, just a few comments. We'll also want to squash the commits to a lower number in the end.

OpenRA.Game/Primitives/BitSet.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Cnc/Activities/Teleport.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Cnc/Traits/Minelayer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/ModelRenderer.cs Outdated Show resolved Hide resolved
@eduherminio
Copy link
Contributor Author

Changes/Ideas look good to me overall, just a few comments. We'll also want to squash the commits to a lower number in the end.

I (think I) addressed all the comments, do you want me to try to logically groups commits or just squash everything when you merge it?

@RoosterDragon
Copy link
Member

For any issues that are fully resolved, can we add the corresponding dotnet_diagnostic.XXYYYY.severity = warning line into .editorconfig so they get flagged automatically in the future?

@abcdefg30
Copy link
Member

do you want me to try to logically groups commits or just squash everything when you merge it?

Ideal would be one commit per "change", i.e. one for .Any() -> .Count or .Length, one for removing the private setters etc. (But if that is too much work for you, I also wouldn't mind if you squashed it down to fewer commits.)

@eduherminio
Copy link
Contributor Author

do you want me to try to logically groups commits or just squash everything when you merge it?

Ideal would be one commit per "change", i.e. one for .Any() -> .Count or .Length, one for removing the private setters etc. (But if that is too much work for you, I also wouldn't mind if you squashed it down to fewer commits.)

Done @abcdefg30, also added @RoosterDragon's suggestion

@abcdefg30
Copy link
Member

Can you squash the space -> tab fixup into the "Use pattern matching" commit (where I think the change first happens)? (So that we don't have random revert commits in the history.) LGTM afterwards.

@eduherminio
Copy link
Contributor Author

Can you squash the space -> tab fixup into the "Use pattern matching" commit (where I think the change first happens)? (So that we don't have random revert commits in the history.) LGTM afterwards.

👍🏼 Squashed and rebased.

Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Sorry, found two more small things.

OpenRA.Mods.Cnc/Traits/Minelayer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Handshake.cs Outdated Show resolved Hide resolved
abcdefg30
abcdefg30 previously approved these changes May 12, 2022
Copy link
Member

@atlimit8 atlimit8 left a comment

Choose a reason for hiding this comment

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

I found one probable issue and a minor nit.

OpenRA.Mods.Cnc/FileFormats/BlowfishKeyProvider.cs Outdated Show resolved Hide resolved
eduherminio and others added 2 commits May 16, 2022 09:35
Co-authored-by: atlimit8 <atlimit8-vcs@gmx.com>
Copy link
Member

@atlimit8 atlimit8 left a comment

Choose a reason for hiding this comment

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

NOTE: I don't know why my comment for this line was not added to my last review; I probably forgot to add it to the review. That is why I have been waiting.

@atlimit8 atlimit8 merged commit a1811b4 into OpenRA:bleed May 18, 2022
@atlimit8
Copy link
Member

Changelog

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.

None yet

6 participants