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

Thoughts to reorganize rules in Performance chapter #246

Open
bkoelman opened this issue Feb 3, 2022 · 1 comment
Open

Thoughts to reorganize rules in Performance chapter #246

bkoelman opened this issue Feb 3, 2022 · 1 comment

Comments

@bkoelman
Copy link
Contributor

bkoelman commented Feb 3, 2022

Not all rules in this chapter are about performance. Some are about multi-threading and async/await:

  • Only use async for low-intensive long-running activities (AV1820)
    This rule is about application responsiveness.
  • Beware of mixing up async/await with Task.Wait (AV1830)
  • Beware of async/await deadlocks in special environments (e.g. WPF) (AV1835)
    These rules are about correctness and attempt to protect against deadlocks.
  • Await ValueTask and ValueTask<T> directly and exactly once (AV1840)
    Also about correctness.

On the other hand, the next rules concern performance, but are in a different chapter:

  • Use generic constraints if applicable (AV1240)
    Aside from code readability, using type parameters with constraints avoids expensive casts and boxing.
  • Evaluate the result of a LINQ expression before returning it (AV1250)
    The point is to avoid doing duplicate work and reuse cached data.
  • Only use the dynamic keyword when talking to a dynamic object (AV2230)
    Dynamic is easy to write but performs poorly.

The next rules affect async/await, but are in a different chapter:

  • Properly handle exceptions in asynchronous code (AV1215)
  • Postfix asynchronous methods with Async or TaskAsync (AV1755)

I'm thinking we should have a chapter on Performance and one on Async/Threading. But of course, that would change the IDs of existing rules, which is problematic for https://www.nuget.org/packages/CSharpGuidelinesAnalyzer/, because developers have existing suppressions by rule ID in their codebases, which will break.

@dennisdoomen
Copy link
Owner

As discussed f2f, it seems that the impact on existing consumers of your analyzer would have to reconfigure their suppressions. It might be better to postpone that until the next major version.

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

No branches or pull requests

2 participants