Implement early bail-out #5410
Implement early bail-out #5410
Conversation
costin-zaharia-sonarsource
force-pushed
the
costin/exit-early-cs
branch
from
247bd74 to
995ebd0
Compare
|
1st round of comments was resolved in #5405 |
| End Class | ||
|
|
There was a problem hiding this comment.
Just FYI (no change needed): This line should not have been removed. There should be empty line before&after Class and Namespace lines
| @@ -46,6 +47,8 @@ public abstract class LocksReleasedAllPathsBase : SymbolicRuleCheck | |||
| "TryEnterWriteLock" | |||
| }; | |||
|
|
|||
| protected abstract ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector); | |||
There was a problem hiding this comment.
To be explicit about the price of the operation. Get should give something that exists.
| protected abstract ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector); | |
| protected abstract ISafeSyntaxWalker CreateSyntaxWalker(LockAcquireReleaseCollector collector); |
|
|
||
| protected sealed class LockAcquireReleaseCollector | ||
| { | ||
| private static readonly HashSet<string> LockTypes = new() { "Mutex" }; // For some APIs ctor can directly acquire the lock (e.g. Mutex). |
There was a problem hiding this comment.
Does this already need a hashset already? A single string should perform way better. Even an array could be a bit faster by not computing the hash code.
| // ToDo: Implement early bail-out if there's no interesting descendant node in context.Node to avoid useless SE runs https://github.com/SonarSource/sonar-dotnet/issues/5375 | ||
| public override bool ShouldExecute() => | ||
| NodeContext.Node.DescendantNodes().OfType<IdentifierNameSyntax>().Any(x => ShouldExecuteFor(x.Identifier)); | ||
| protected override ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector) => new LockAcquireReleaseWalker(collector); |
There was a problem hiding this comment.
Nitpick: It's not a property and body is not trivial. Should be on the next line
| protected override ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector) => new LockAcquireReleaseWalker(collector); | |
| protected override ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector) => | |
| new LockAcquireReleaseWalker(collector); |
| // ToDo: Implement early bail-out if there's no interesting descendant node in context.Node to avoid useless SE runs https://github.com/SonarSource/sonar-dotnet/issues/5375 | ||
| public override bool ShouldExecute() => | ||
| NodeContext.Node.DescendantNodes().OfType<IdentifierNameSyntax>().Any(x => ShouldExecuteFor(x.Identifier)); | ||
| protected override ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector) => new LockAcquireReleaseWalker(collector); |
There was a problem hiding this comment.
Same nitpick, body should be on the next line
costin-zaharia-sonarsource
force-pushed
the
costin/exit-early-cs
branch
from
4fa899d to
a7c71c0
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed! |
Fixes #5375