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

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

Merged
merged 5 commits into from
Aug 25, 2023

Conversation

danmar
Copy link
Owner

@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.

@@ -67,6 +67,18 @@
#include <windows.h>
#endif

static const std::set<std::string> criticalErrors{
Copy link
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
Owner 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
Owner 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
Owner 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
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
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
Owner 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
Owner 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
Owner 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
Owner 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
142 checks passed
@danmar danmar deleted the fix-11892 branch August 25, 2023 11:38
@firewave
Copy link
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