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

mitigated Unchecked return value Coverity warnings #5980

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

@pfultz2 please have a look.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

ok so as far as I see these are false positives.. we ignore the return value by intention?

another possible mitigation would be to mark them as false positives in the coverity ui right? however it could mean more work in the long run if we need to mark them as false positives again..

@firewave
Copy link
Collaborator Author

These are not a false positives. The analysis looks at the code and if it sees that the return value is used in most cases it flags it as unintentional and a potential issue. If we would enable the clang-tidy warning about introducing [[nodiscard]] that would probably come up as well.

@danmar
Copy link
Owner

danmar commented Feb 26, 2024

These are not a false positives. The analysis looks at the code and if it sees that the return value is used in most cases it flags it as unintentional and a potential issue.

well.. from my point of view that ARE false positives. If we ignore the return values by intention then they are false positives. If they are true positives (we unintentionally ignore the result) then you shouldn't add void casts.

I don't classify a finding as false positive or true positive depending on what heuristics the tool use. The heuristic coverity uses is not insane at all. I do feel it could probably detect some genuine bugs also.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

I approve this fix..

@firewave
Copy link
Collaborator Author

I will still wait for feedback from @pfultz2 as I have no idea if this was intentional or not.

@orbitcowboy
Copy link
Collaborator

I will still wait for feedback from @pfultz2 as I have no idea if this was intentional or not.

I vote to merge this because it is good practice to be explicit. By casting a return value to void, its clear that the return value is ignored in this context.

@firewave
Copy link
Collaborator Author

I vote to merge this because it is good practice to be explicit. By casting a return value to void, its clear that the return value is ignored in this context.

Watch out for more fun in that area soon.

@firewave firewave merged commit 2626e2e into danmar:main Apr 2, 2024
64 checks passed
@firewave firewave deleted the cov-unchecked branch April 2, 2024 18:06
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

Successfully merging this pull request may close these issues.

3 participants