CI-unixish.yml: removed random usage of build dir from selfcheck #6938
Conversation
|
This is not providing anything useful. I filed https://trac.cppcheck.net/ticket/13245 about about integrating this properly. |
.github/workflows/CI-unixish.yml
Outdated
| mkdir b1 | ||
| ./cppcheck $selfcheck_options $cppcheck_options --cppcheck-build-dir=b1 --addon=naming.json cli || ec=1 | ||
| ./cppcheck $selfcheck_options $cppcheck_options --cppcheck-build-dir=b1 --addon=naming.json --enable=internal lib || ec=1 | ||
| ./cppcheck $selfcheck_options $cppcheck_options --addon=naming.json cli || ec=1 |
There was a problem hiding this comment.
I was not aware of that.
But that kind of supports the case that it seems "random" and we need to properly integrate it. So we should probably also add it in daca.
I assume that we also lack test coverage for this.
There was a problem hiding this comment.
It could be that it's not well tested nor well documented. But that is no reason to remove ctu checkers from our selfchecks. I would still want to get such warnings.
Now that we have the ctu information for addons in memory maybe we can try to migrate the internal ctu info to the same memory also. And then the build dir will not be required to get multithreaded whole program analysis.
There was a problem hiding this comment.
It could be that it's not well tested nor well documented. But that is no reason to remove ctu checkers from our selfchecks. I would still want to get such warnings.
Well, I filed a ticket about implementing it in a better way (and we could even leverage the build dir in the CI).
Now that we have the ctu information for addons in memory maybe we can try to migrate the internal ctu info to the same memory also. And then the build dir will not be required to get multithreaded whole program analysis.
That sounds interesting but it is not something I want to look into until the suppressions actually work fully (still waiting for feedback on #7346 so I can proceed - we should be close) and the shared execution code is in use. And we also need to merge #7079 first so we actually have proper working test coverage for this.
This is because all that plays into how the analyzer/ctu info is being tracked right now and I would not like to do any changes to that before proper coverage of that.
There was a problem hiding this comment.
Well, I filed a ticket about implementing it in a better way (and we could even leverage the build dir in the CI).
I want that we use the CTU analysis in selfchecks. I don't want that you turn it off now. Maybe this PR should be merged after the reimplementation..
There was a problem hiding this comment.
I want that we use the CTU analysis in selfchecks. I don't want that you turn it off now. Maybe this PR should be merged after the reimplementation..
If you want it then it should be properly implemented i.e. used across the whole code. Since we use multiple invocations I am not sure if it would actually work as intended (one of the reasons I want to drop it). The selfcheck should rather use a compilation database as input (including the ones in the sanitizer workflows) and the existing CLI input could be moved into the proposed workflow in #5477.
There was a problem hiding this comment.
Since we use multiple invocations I am not sure if it would actually work as intended (one of the reasons I want to drop it).
This is true. We will not detect ODR violations for instance if the classes are defined in different folders that are scanned separately and not included.
But we can still detect ODR violations if the classes are defined in the same folder.
I do not believe in preventing that ODR violations are found at all in our selfcheck. If you want to tweak the invocations in CI so that we find more ODR violations properly feel free to do it..
firewave
force-pushed
the
selfcheck-builddir
branch
from
a1f6296 to
13e84cc
Compare
2 participants
No description provided.