Fix #12071 (Add safety mode that makes cppcheck more strict about critical errors) #5777
Conversation
|
@firewave here is another attempt to add more safety compliant analysis. It's now intended to be opt-in. |
|
If it would be nice if there would be at least a 24-72 hour grace period on PRs so people actually have a chance to comment on them before they are being merged... |
| @@ -1066,6 +1066,10 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a | |||
| #endif | |||
| } | |||
|
|
|||
| // Safety certified behavior | |||
| else if (std::strcmp(argv[i], "--safety") == 0) | |||
There was a problem hiding this comment.
There is no unit test for this option in TestCmdlineParser.
There is no documentation either but I do like that because that still allows us to modify this.
I think this should be --safety-exitcode (or rather --critical-exitcode) instead. That would be more alone options we already have and allows people (rather scripts) to differentiate between the type of the failure.
There was a problem hiding this comment.
It's not just about exitcode anymore. It's to enable "safety certified behavior". I envision that this option will not be used much except in our testing. Cppcheck Premium users will get a safety configuration in the cppcheck.cfg file.
| } | ||
|
|
||
| if (msg.severity == Severity::internal) |
There was a problem hiding this comment.
We already have internal errors. So this introduces some ambiguity.
There was a problem hiding this comment.
ok if you think the name is unfortunate I am not against changing it.
These are messages that the user should never see. They will only be reported internally inside cppcheck. That was my intention with the name.. we have 2 use cases currently:
- internal logging of executed checkers
- suppressed critical errors
More use cases could popup later.
| if (mSettings.safety && ErrorLogger::isCriticalErrorId(msg.id)) { | ||
| mExitCode = 1; | ||
|
|
||
| if (mSettings.nomsg.isSuppressedExplicitly(errorMessage, mUseGlobalSuppressions)) { |
There was a problem hiding this comment.
I don't like that we just extended the suppression matrix by factor 2 - it's already way too complex and hard (impossible to me) to understand in code.
There was a problem hiding this comment.
It is complex for sure already. :-(
It adds complexity but I fear that to pass a safety certification we can't allow that users suppress critical errors by mistake.
| @@ -44,3 +44,6 @@ Other: | |||
| - Markup files will now be processed after the regular source files when using multiple threads/processes (some issues remain - see Trac #12167 for details). | |||
| - Added file name to ValueFlow "--debug" output. | |||
| - Fixed build when using "clang-cl" in CMake. | |||
|
|
|||
| Safety critical fixes: | |||
| - Added "--safety" option. It makes Cppcheck more strict about critical errors. | |||
There was a problem hiding this comment.
That should not be in the release notes as it is not documented.
If we make this official we cannot change this without a transition period and makes it harder to adjust it later on.
There was a problem hiding this comment.
I agree. hmm.. I will write this in the proper Cppcheck Premium release notes instead.
I am trying to satisfy conflicting requirements. I can understand that from your point of view this is a quick hack that is not needed. :-(
For Cppcheck Premium the safety certification is very important so we need to satisfy certain requirements, but at the same time I really don't want to make it difficult for existing open source users. Keeping existing open source users happy is also business critical imho.
|
Looking at all the hacks we have to add along the way this doesn't look like a good change. It's mostly rather just that - a hack. I get that it serves a purpose but in this way it can only do this temporally so it should not be documented at all. |
|
I assume this is mainly a change for premium. And I am wondering if this functionality could simply be achieved by introducing the As mentioned in my previous comments on the original commit I am not seeing much value in this functionality for regular users as those critical errors would include things they cannot fix by themselves as they might also include things which indicate bugs within Cppcheck. So more reason not to make this visible. That needs some reworking but that's yet another rabbit-hole to go down into. |
Sorry.. it was not to avoid your review. As you probably understood I am trying to push out this release.. not at all costs of course, I do not want to introduce last minute bugs now. There are customers waiting for the release and there are post release tasks for customers I am very eager to start working on. To me 2.13 does not feel perfect from a quality point of view but it's much better than 2.12. We will continue to work on it and 2.14 should be better.. |
No description provided.