Add a --check-level option

[danmar]

No description provided.

[firewave]

I don't like this at all. This seems way too arbitrary and rushed. At least they should not be documented because that would make it harder to change the behavior without some additional handling or proper deprecation.

As this is also a new "grouping" option it overrides other options and is overwritten by others which is not ideal and makes things harder to tune or understand. We should not be having such options see --enable=style.

We should collect information from daca first (which is also why I think this should opt-in) and then do some tests on much impact changing these values actually have.

[danmar]

I don't like this at all. This seems way too arbitrary and rushed. At least they should not be documented because that would make it harder to change the behavior without some additional handling or proper deprecation.

I think this is a misunderstanding.. we do not define what exactly --check-level=normal|exhaustive does technically. It's flexible. We can have different tradeoffs in Cppcheck-2.11 and Cppcheck-2.12.. etc..

We will implement the tradeoffs that we think are most effective to accomplish what the user wants.

[danmar]

As this is also a new "grouping" option it overrides other options and is overwritten by others which is not ideal and makes things harder to tune or understand. We should not be having such options see --enable=style.

I have hidden the option --performance-valueflow-max-if-count so it's not official. Well I don't know if we need that option at all.

For now.. I think it's an undocumented option. I don't promise that we will have it in future releases.

[danmar]

We should collect information from daca first (which is also why I think this should opt-in) and then do some tests on much impact changing these values actually have.

Why? I see no reason to do it first. I mean I really like the idea to collect more information from daca and tweak the check level parameters. Definitely I will do it. But it's not something we have to decide right now. We can throw in some initial tradeoffs now and then iterate and make it better and better..

[firewave]

For now.. I think it's an undocumented option. I don't promise that we will have it in future releases.

I have no problem with such options being official but some of them are currently quite fidgety and just serves a single purpose. They are not balanced and easy to use at all (if you actually want to use them properly and not just set-and-forget to get rid of the performance problem). And we already have too much functionality which is not working properly or failing/bailing silently and we should not add on top of that. So keeping it "unofficial" makes it easier to adjust it to get it right.

Why? I see no reason to do it first. I mean I really like the idea to collect more information from daca and tweak the check level parameters. Definitely I will do it. But it's not something we have to decide right now. We can throw in some initial tradeoffs now and then iterate and make it better and better..

The problem is that we do not provide and collect all the information necessary to make such decisions. So if you just throw some new default out there, you don't have any baseline to compare with. If you introduce it softly with the information collected the initial decision is much more informant and you are not required to do additional work.

Just a single top-level issue with data collecting I haven't even thought about before:
In daca the options are prepared by the client and until it has updated you are not getting the same data from all clients making it hard to introduce something like different defaults. We probably need to move the fixed part of the option onto the server so there's less logic in the client. And even then all the client need to be updated first so they all get those options. So it takes quite a while until you are even properly collecting data.

I have an idea on how to do that but as mentioned several times before I don't have the capacity to get into that right now. And like most of the things I am currently dealing with it's the more you look into it the more you see things you need to improve/change before you even get a single step closer to the solution...

[pfultz2]

LGTM!

[danmar]

The problem is that we do not provide and collect all the information necessary to make such decisions. So if you just throw some new default out there, you don't have any baseline to compare with. If you introduce it softly with the information collected the initial decision is much more informant and you are not required to do additional work.

It is true I don't know the exact effect on all the projects however I do know for certain that the effect is dramatic on some projects. Intuitively I don't understand why the performance would be much worse for any projects.

I did not use Cppcheck to self check Cppcheck Premium in the github action because it took so much time. It would have prevented work. Now I have started using Cppcheck.

[firewave]

I did not use Cppcheck to self check Cppcheck Premium in the github action because it took so much time. It would have prevented work. Now I have started using Cppcheck.

I don't know why 3 minutes in the selfcheck are such an issue. Overall that doesn't make much of a difference compared to most of the other CI jobs.

But having the file open in an IDE with Cppcheck for highlighting - that's unacceptable. Although it's an exception like I mentioned before. clangd/clang-tidy are much, much slower than Cppcheck on almost any other file.

[danmar]

I don't know why 3 minutes in the selfcheck are such an issue. Overall that doesn't make much of a difference compared to most of the other CI jobs.

I did not talk about "Cppcheck Open Source CI" but "Cppcheck Premium CI". It had a big impact on the overall time.

[danmar]

For information it saves ~10 minutes in Cppcheck Premium:

daniel@laptop:~/cppchecksolutions/addon$ time ~/cppcheck/cppcheck --check-level=normal src/checkautosar.cpp 
Checking src/checkautosar.cpp ...

real    0m1,424s
user    0m1,412s
sys     0m0,012s
daniel@laptop:~/cppchecksolutions/addon$ time ~/cppcheck/cppcheck --check-level=exhaustive src/checkautosar.cpp 
Checking src/checkautosar.cpp ...

real    10m11,475s
user    10m11,406s
sys     0m0,021s

The overall time is 2 minutes now. Compiling+unit tests+cppcheck check => 2 minutes.

[danmar]

@firewave As I read it.. you are not against normal and exhaustive mode itself. You are against blunt fixes for normal checking. I can understand this and agree. In this PR I add the checking mode but it does not introduce any blunt tweaks.

I merge this so people can hopefully start separating further analysis into normal and exhaustive checking.

You've said that cppcheck-1.90 or so was pretty fast. How about using that as a baseline then for "normal" checking. I would not be against that with "normal" in future releases you more or less get the cppcheck-1.90 analysis but with all the bug fixes and any improvements that are quick enough.

[danmar]

And we already have too much functionality which is not working properly or failing/bailing silently

In my opinion we must have limits. If we remove all limits and bailouts then cppcheck will get much slower.. even exhaustive analysis must be able to finish in some reasonable time.

[firewave]

@firewave As I read it.. you are not against normal and exhaustive mode itself. You are against blunt fixes for normal checking. I can understand this and agree. In this PR I add the checking mode but it does not introduce any blunt tweaks.

Correct. My approach is to be strict/secure/complete out-of-the-box but provide all the options to "shoot yourself in the foot". It's also due diligence as somebody has to manually set an option to change the "proper" behavior and you cannot be blamed for it being insecure or false positives/negatives.

You've said that cppcheck-1.90 or so was pretty fast. How about using that as a baseline then for "normal" checking. I would not be against that with "normal" in future releases you more or less get the cppcheck-1.90 analysis but with all the bug fixes and any improvements that are quick enough.

The main difference is that the ValueFlow was not as advanced. Hence me adding an environment variable to turn that off and trying to make its execution lazy in #4521 (I will re-visit that PR soon). If you have that figure out why it might still be called and try to optimize that code. Afterwards see what is the most expensive part of the ValueFlow and make that optional/lazy, rinse, repeat. That also allows you to properly profile the non-hot spot parts and find things to improve there.

I guess my main offense was that we jump on the hot spot and try to arbitrarily ticker with that in terms which goes above a simple "hot fix". But we don't have to discuss this anymore.

Beside that the main false negatives from the changes in that time frame should get some major eyeballs on them IMO. I guess it would also be interesting to run selfchecks with older releases to see if there are false negatives on our own code. You could even make that a matrix by also using the source from older releases as well.

I looked into the compilation speed and it seems (it's hard to tell even with the data from the ClangBuildAnalyzer logs) that is mainly caused by the increase of containers in library.h which is pulled in via settings.h (some basically the whole source). There's probably some containers which use the same key and could actually be merged (e.g. 'mMarkupExtensions', 'mReportErrors' and mProcessAfterCode). The question is how this might affect run-time performance though.

FYI I filed https://trac.cppcheck.net/ticket/11671 and https://trac.cppcheck.net/ticket/11672 about ways to improve performance in terms of the generated code.

[firewave]

An (undocumented) unlimited would be good. That might also help to find some bugs where e.g. we end up in an infinite loop but it would not affect the performance.