Fix #10039 (integrate --check-config include findings with normal analysis) / also fixes #11283

[firewave]

No description provided.

[amai2012]

I like the Idea.
But I would prefer to combine it with a necessary major CLI overhaul...

[firewave]

I like the Idea.
But I would prefer to combine it with a necessary major CLI overhaul...

Unfortunately I don't have the time for that. And I still need to integrate this change with the GUI (which I never used so far) and my pending changes to the CLion plugin.

Is there a ticket outlining this "necessary" overhaul? If not, could you please create one? Thanks.

[amai2012]

@firewave There has been some discussion but I wasn't able to dit out a suitable trac ticket.

Maybe we should focus on the details instead: so missingInclude messages should now show up in normal analysis? Is there a notable impact on performance?

[firewave]

Maybe we should focus on the details instead: so missingInclude messages should now show up in normal analysis? Is there a notable impact on performance?

Not that I am aware of.

The checks are performed anyways but it doesn't show the detailed information of which includes are actually missing. Just the generic one that some are missing and you should run it with --check-config to get that information.

[firewave]

I figured it makes more sense to just report the multiple messages in the normal analysis instead of a message which tells you to run --check-config. If you are interested in those findings you want to know the details and having to do a roundtrip with a different call makes no sense at all. It's also a way less intrusive change.
And regarding any performance impact - if there is any it will be negligible compared to the all (massive) performance regressions which have been added in the past release cycles.

This also would limit --check-config to do what it actually indicates to do. Just checking the configurations.

[danmar]

maybe. does this encourage users to include all system paths? that would be unfortunate. can users test out cppcheck on their code without getting a flood of warnings about missing includes. Personally I would primarily suggest that --project is used if possible.

And regarding any performance impact

I also think we can neglect this

[firewave]

maybe. does this encourage users to include all system paths? that would be unfortunate. can users test out cppcheck on their code without getting a flood of warnings about missing includes. Personally I would primarily suggest that --project is used if possible.

missingInclude is disabled by default and if you turn it on you get a (pretty useless IMO) message and you have to run --check-config and that will all the messages - local and system includes. This makes things complicated if you want to automate this (i.e. CI or IDE integration).

Maybe we need to introduce a missingSystemInclude check to configure. We don't need those since it is covered by the --library option and if you configure it we will probably run into more trouble than if you hadn't. Having a missing local include will lead to false positives and negatives. And having it in the same results makes it easier for IDE integrations to report misconfigurations.

That's where the idea came from. The CLion plugin is invoking the files without any proper context (since it is either not available or hard to obtain through the Plugin API) and you do not get any feedback about which include is actually missing.

I would consider missingInclude similar to preprocessor errors since it's a misconfiguration of the analysis that should not be hidden away if you are doing a scan and should not even be an information message. I do get that it can be hard if there are generated headers or some heavy dependencies. We jump through hoops to avoid an additional compilation of everything to keep the CI fast. CMake has Cppcheck support built in but that performs a complete build and invokes Cppcheck next to it.

Going a bit off-topic:
I am also thinking about a --disable= or --enable=none since it is really hard to get rid of the default checks. And also making error configurable as well as introducing fatal which would refer to analysis breaking findings like syntaxError etc. Not having this is actually causing severe slowdowns if you are just running unusedFunction since we will still perform all default (i.e. style) as well as ValueFlow and all the non-conditional (bogus and legitimate) checks which are all unnecessary (except for the suggested fatal group which would cause false positives/negatives).
Having these options would also help to so some more targeted profiling with having only certain things actually enabled. (For instance most of the speed regressions we added recently seem to be related to the ValueFlow stuff).

As usual I will try to be incremental about those things but it will probably not happen soon - I have spent too much here during my "ping-pong-ing". I will try to file tickets about it though.

[danmar]

Having a missing local include will lead to false positives and negatives. And having it in the same results makes it easier for IDE integrations to report misconfigurations.

I agree of course.

since it is either not available or hard to obtain through the Plugin API

hmm.. I was assuming that in a IDE (plugin), it would be easy to get proper compiler configurations.

[danmar]

CMake has Cppcheck support built in but that performs a complete build and invokes Cppcheck next to it.

sorry you probably know it already .. but can you generate the compile_commands.json with cmake ? then you can run cppcheck on a single file..

[firewave]

hmm.. I was assuming that in a IDE (plugin), it would be easy to get proper compiler configurations.

It probably depends. The IDEA plugin is not CLion dependent but for all IDEs so it doesn't use the available CLion integration. I have not been able to build such a plugin yet and the documentation is really bad to non-existent. Beside it being buggy as hell. I got in touch with a developer and filed some bugs but nothing has come from it yet. Also me not having the time to look into it anymore isn't helping either. The plugin only having a global and no project-specific configuration makes things a bit harder.

sorry you probably know it already .. but can you generate the compile_commands.json with cmake ? then you can run cppcheck on a single file..

I am not sure what you mean. Do you mean for the IDE analysis?

Also you don't actually need CMake to generate a compilation database. You just need clang as compiler and add the -MJ flag. So you could also generates those from makefiles. See https://clang.llvm.org/docs/ClangCommandLineReference.html#dependency-file-generation and https://sarcasm.github.io/notes/dev/compilation-database.html#clang.

[danmar]

I am not sure what you mean. Do you mean for the IDE analysis?

if there is a compile database then the plugin can execute:

cppcheck --project=compile_commands.json  --file-filter=thefile.c 

to just check thefile.c

[firewave]

I am not sure what you mean. Do you mean for the IDE analysis?

if there is a compile database then the plugin can execute:

cppcheck --project=compile_commands.json  --file-filter=thefile.c 

to just check thefile.c

Ah, now I get you. That was exactly what I wanted to do but I could not figure out how to get the build directory from the IDE.

I am finally getting some more feedback on how a plugin actually needs to be written and have another developer contact which I can use when I get into the development again.

[firewave]

This still needs a couple of unit tests before it is ready.

[firewave]

This also fixes https://trac.cppcheck.net/ticket/11283 which was essentially broken in non-Windows binaries since ages.

[firewave]

missingInclude and missingIncludeSystem findings do not affect the exitcode.

--check-library and unmacthedSuppression for some reason do affect it although they are also information messages, so I think that is a bug.

[firewave]

On a side note:
The last remaining use of reportInfo() is for ConfigurationNotChecked. IMO we should fail on every finding when --error-exitcode is specified otherwise the option is kind of useless since you use the exitcode for automated execution where you don not want or even cannot review the output.

That would also allow use to remove that method from the interface which will clean up things and possibly help with other things (I think this once caused me some problems in some refactoring).

[danmar]

The last remaining use of reportInfo() is for ConfigurationNotChecked.

I have the feeling we added reportInfo() so those messages can be displayed differently in the GUI.

IMO we should fail on every finding when --error-exitcode is specified otherwise the option is kind of useless since you use the exitcode for automated execution where you don not want or even cannot review the output.

I think that sounds OK to me.

[firewave]

The last remaining use of reportInfo() is for ConfigurationNotChecked.

I have the feeling we added reportInfo() so those messages can be displayed differently in the GUI.

That might possibly be reportOut().

[danmar]

I think for some users it will be more visible that some local headers must be included that is good!

Some users will start including every header, including stdio.h etc.. that is not good. I believe we will need to try to prevent that better. This message Include file: <stdio.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. is unfortunate imho. It would be better to suggest --library=std directly. If it's not known which library has the header then it's better to recommend --library somehow anyway.

[firewave]

I think for some users it will be more visible that some local headers must be included that is good!

Some users will start including every header, including stdio.h etc.. that is not good. I believe we will need to try to prevent that better. This message Include file: <stdio.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. is unfortunate imho. It would be better to suggest --library=std directly. If it's not known which library has the header then it's better to recommend --library somehow anyway.

Good point. When I split missingInclude and missingIncludeSystem into separate checks I will adjust the message.

[danmar]

@firewave thanks.. I tried to tweak the messages in PR #4850 please give me some feedback on that.