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

some CppCheck and related usage cleanups #5979

Merged
merged 6 commits into from
Feb 19, 2024
Merged

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

This is a continuation of the CheckUnusedFunctions refactoring but also works towards #4964 and might also tie in with the executor refactoring.

This change is already buried somewhere in my countless branches but I did the changes from scratch now that I encountered this again.

@firewave firewave marked this pull request as draft February 12, 2024 19:37
* and if it's possible at all */
bool isUnusedFunctionCheckEnabled() const
{
return useSingleJob() && checks.isEnabled(Checks::unusedFunction);
Copy link
Owner

Choose a reason for hiding this comment

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

hmm.. I wonder why it doesn't check useSingleJob() || !buildDir.empty()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just keeping the current behavior intact for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unsurprisingly tis check turned out flawed. This is addressed in #6207.

if (mSettings.checks.isEnabled(Checks::unusedFunction) &&
mSettings.useSingleJob() &&
mSettings.buildDir.empty()) {
if (mSettings.isUnusedFunctionCheckEnabled() && mSettings.buildDir.empty()) {
Copy link
Owner

Choose a reason for hiding this comment

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

hmm.. I don't understand why we required that the buildDir is empty. Don't we want to run this checking here for some reason if the build dir is empty..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea. Currently I am keeping the current behavior intact. I will look into this when I look at the build directory related false positives. But that requires adding a lot more general testing of the build directory first.

@firewave
Copy link
Collaborator Author

firewave commented Feb 12, 2024

Okay - now I do recall why those changes were buried in the first place because I encountered exactly these testing failures.

@firewave
Copy link
Collaborator Author

Okay - now I do recall why those changes were buried in the first place because I encountered exactly these testing failures.

I was passing the wrong suppressions to the reporting function. Should be fine now.

@firewave firewave marked this pull request as ready for review February 12, 2024 21:45
@firewave firewave marked this pull request as draft February 13, 2024 03:53
@firewave firewave marked this pull request as ready for review February 13, 2024 04:04
@firewave
Copy link
Collaborator Author

Merging as there has been no feedback for almost a whole week. It is also blocking traffic as usual.

@firewave firewave merged commit d429d7d into danmar:main Feb 19, 2024
64 checks passed
@firewave firewave deleted the cppcheck branch February 19, 2024 19:24
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.

2 participants