Skip to content

Partial fix #11892 (Safety: status message when checkers are skipped)#5368

Merged
danmar merged 5 commits intomainfrom
fix-11892
Aug 25, 2023
Merged

Partial fix #11892 (Safety: status message when checkers are skipped)#5368
danmar merged 5 commits intomainfrom
fix-11892

Conversation

@danmar
Copy link
Copy Markdown
Collaborator

@danmar danmar commented Aug 24, 2023

Work in progress to be more visible when there are critical errors, and no checking is executed in a file.

Comment thread cli/cppcheckexecutor.cpp Outdated
#include <windows.h>
#endif

static const std::set<std::string> criticalErrors{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What about introducing a fatal severity instead?

Should also include missingInclude as incomplete code will also lead to incomplete analysis.

Copy link
Copy Markdown
Collaborator Author

@danmar danmar Aug 25, 2023

Choose a reason for hiding this comment

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

I do not agree about adding missingInclude. We will always have false negatives. Our parser is far from perfect. I will never promise that we have "complete" analysis.

I would normally include all local headers when I check some code.

More include paths are not always better. It can have a significant performance penalty. System headers should not be included in general. And I have the feeling that if the header code is very complex it can be better to leave it out.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

For the error ids I listed, the analysis is stopped. The checkers are not executed at all.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

About adding a fatal severity. In theory it sounds good to me. It's just that it's one more thing that will break plugins and scripts.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just off the top of my head (it's based on something I considered for another improvement).

  • add Severity::fatal and change the errors in question
  • add Settings::fatalErrors (default: false)
  • and command-line option --fatal-errors
  • add support for fatal to --enable/--disable
  • if that option is not set treat them simply as error
  • always enable that option for the GUI analysis

This way you can opt-in/opt-out and it fits in the existing frameworks.

@firewave
Copy link
Copy Markdown
Collaborator

This should be configurable as it will most certainly break existing installations which have suppressed these errors (for a long time possibly).

@danmar danmar marked this pull request as draft August 25, 2023 06:39
@danmar
Copy link
Copy Markdown
Collaborator Author

danmar commented Aug 25, 2023

This should be configurable as it will most certainly break existing installations which have suppressed these errors

I believe that if such errors are suppressed then my changes will not change the behavior of cppcheck.

@danmar
Copy link
Copy Markdown
Collaborator Author

danmar commented Aug 25, 2023

@firewave After your questions I am not sure if this is the right approach. At least not in the command line. Thanks!

@danmar danmar closed this Aug 25, 2023
@danmar
Copy link
Copy Markdown
Collaborator Author

danmar commented Aug 25, 2023

hmm.. but I guess the GUI changes makes sense at least to start with..

@danmar danmar reopened this Aug 25, 2023
@danmar
Copy link
Copy Markdown
Collaborator Author

danmar commented Aug 25, 2023

@firewave maybe it would make sense to treat missingInclude, varid0, checkLibrary, debug warnings, etc differently if the check level is "exhaustive"?

@danmar danmar marked this pull request as ready for review August 25, 2023 11:33
@danmar danmar changed the title Fix #11892 (Safety: status message when cppcheck analysis fails) Partial fix #11892 (Safety: status message when cppcheck analysis fails) Aug 25, 2023
@danmar danmar changed the title Partial fix #11892 (Safety: status message when cppcheck analysis fails) Partial fix #11892 (Safety: status message when checkers are skipped) Aug 25, 2023
@danmar danmar merged commit 74ad724 into main Aug 25, 2023
@danmar danmar deleted the fix-11892 branch August 25, 2023 11:38
@firewave
Copy link
Copy Markdown
Collaborator

@firewave maybe it would make sense to treat missingInclude, varid0, checkLibrary, debug warnings, etc differently if the check level is "exhaustive"?

We can only include missingInclude without additional logic. Everything else needs additional logic or features otherwise you will be drowning in false positives you will not be able to fix. I will try to file tickets about this in the next days. It also involves adding some new data collection and reports to daca first. (If I were in a better condition, all of that would have been implemented already - probably...but I guess I am a bit overly optimistic 🫤)

I thought about the same. Making fatal errors non-fotal with exhaustive. But that seems a bit too obscure. Using the existing framework would be better.

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