CalculationsShouldNotOverflow.SyntaxKindWalker reduce allocations and evaluations in the hot path #8133
Conversation
analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/CalculationsShouldNotOverflow.cs
Outdated
Show resolved
Hide resolved
...zers/tests/SonarAnalyzer.UnitTest/Rules/CalculationsShouldNotOverflowSyntaxKindWalkerTest.cs
Outdated
Show resolved
Hide resolved
...zers/tests/SonarAnalyzer.UnitTest/Rules/CalculationsShouldNotOverflowSyntaxKindWalkerTest.cs
Outdated
Show resolved
Hide resolved
...zers/tests/SonarAnalyzer.UnitTest/Rules/CalculationsShouldNotOverflowSyntaxKindWalkerTest.cs
Outdated
Show resolved
Hide resolved
...zers/tests/SonarAnalyzer.UnitTest/Rules/CalculationsShouldNotOverflowSyntaxKindWalkerTest.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| [DataTestMethod] | ||
| [DataRow("_ = 1 + 1;", true)] // Fix: should be false as it is checked at compile time |
There was a problem hiding this comment.
What does the "Fix" mean here? I don't get it
There was a problem hiding this comment.
These are all cases with a binary operation but never an overflow. This is the case for constants (checked by the compiler) and string concatenation. The idea was to document the cases here and add an issue to fix later. But I wanted to know if there was enough value in adding it.
There was a problem hiding this comment.
Unless we have FPs reported, I would not add issue for that.
Let's remove the Fix: prefix, the Should .... is enough to explain the problem
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
martin-strecker-sonarsource
force-pushed
the
Martin/ShouldNotOverlflow_Refactoring
branch
from
eb95d8e
to
a5bd2aa
Compare
| """)).Tree; | ||
| var sut = new CalculationsShouldNotOverflow.SyntaxKindWalker(); | ||
| sut.SafeVisit(tree.GetRoot()); | ||
| sut.HasOverflow.Should().Be(false); |
There was a problem hiding this comment.
There're dedicated methods for true and false. Same below.
| sut.HasOverflow.Should().Be(false); | |
| sut.HasOverflow.Should().BeFalse(); |
| [DataRow("""_ = "" + s + s;""", true)] // Fix: should be false as one of the operands is a string literal | ||
| [DataRow("""_ = s + "" + s;""", true)] // Fix: should be false as one of the operands is a string literal | ||
| [DataRow("""_ = s + s + "";""", true)] // Fix: should be false as one of the operands is a string literal | ||
| public void HasOverflowExpressions_Literals(string statement, bool expected) |
There was a problem hiding this comment.
I would remove the bool expected, as it's always true. If False is needed, second method can be added, or parameter can be re-added later.
There was a problem hiding this comment.
This misses a bit the purpose of the tests: Enable an incremental optimization by turning these into false. I kept it this way.
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed! |
martin-strecker-sonarsource
changed the title
martin-strecker-sonarsource
deleted the
Martin/ShouldNotOverlflow_Refactoring
branch
|
Peach validation: We lost a couple of issues that have an unchecked expression, that wasn't detected before. |
Fixes #8103