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

Fix S2583/2589 FP: Return inside lock, using and finally causes FP after the block #8761

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

pavel-mikula-sonarsource
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource commented Feb 15, 2024

Fixes #8678
Fixes #8495

There's one commit that does the raw string literal clenup in isolation. The actual change is small

Copy link

sonarcloud bot commented Feb 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Feb 15, 2024

var current = FinallyPoint;
while (current is not null && other is not null)
{
if (current.BlockIndex == other.BlockIndex && current.BranchDestination == other.BranchDestination)

Choose a reason for hiding this comment

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

Suggestion: Could this line be an equals override in FinallyPoint?
And then you just check current.Equals(other)?
Looking at the codebase, this is the only point that you ever compare finally points.
Or more correctly maybe, the entire method should be inside Equals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial implementation and I didn't go that way, because this logic is strictly bound to what ExplodedNode needs.

Such Equals would ignore that the branches are different, and are possibly from different source blocks. So I didn't want to use Equals for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Logic and testing, it looks good.
I am not sure I understand how finally points work.
I would suggest asking for Tim for a second round, just to be sure.

Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

LGTM!

@Tim-Pohlmann Tim-Pohlmann merged commit afd7543 into master Feb 16, 2024
36 checks passed
@Tim-Pohlmann Tim-Pohlmann deleted the Pavel/S2583-FinallyPoint branch February 16, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants