-
Notifications
You must be signed in to change notification settings - Fork 223
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
SE: ComputeBoolConstraint for numeric comparisons #7176
Conversation
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs
Show resolved
Hide resolved
fa00159
to
319f219
Compare
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 overall. Just some minor comments and questions.
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs
Show resolved
Hide resolved
.../tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs
Show resolved
Hide resolved
...rAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.Loops.cs
Show resolved
Hide resolved
6f4339f
to
7e9dcc5
Compare
Kudos, SonarCloud Quality Gate passed! |
SonarCloud Quality Gate failed. |
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.
LGTM. Added a single test case.
NumberConstraint.From(1, 1).IsSingleValue.Should().BeTrue(); | ||
NumberConstraint.From(null, null).IsSingleValue.Should().BeFalse(); | ||
NumberConstraint.From(null, 42).IsSingleValue.Should().BeFalse(); | ||
NumberConstraint.From(42, null).IsSingleValue.Should().BeFalse(); |
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.
I know coverage is not a requirement for this PR, but this makes sense to test.
NumberConstraint.From(42, null).IsSingleValue.Should().BeFalse(); | |
NumberConstraint.From(42, null).IsSingleValue.Should().BeFalse(); | |
NumberConstraint.Empty.IsSingleValue.Should().BeFalse(); |
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.
From(null, null)
is already tested, so Empty
will not change anything.
I don't really know what that line is not covered, we have
HasValue==true
HasValue==false
Min==Max
Min!=Max
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.
Oh! I think it's missing From(1, 42)
.
Additional note: I just changed the behavior of From(null, null)
in #7174. You should change the test to use Empty
instead.
Fixes #7142
Fixes #2528
I'm not going to fix the coverage, because we need #7143 first