Let's try to make cppcheck work again, and even report useful stuff.
During the journey it found a few actual issues, which is good. It also found some false-positives, which isn't so good. We probably could reduce the false positives by not using `--library=gtk`, but that also would reduce the actual issues it can find.
There are a few issues that should be reported upstream, but it's harder than it looks… if I could only remember my password on their tracker :confused:
Anyway, what do you guys think? Is it good? Are there too many false-positives? Should the suppressions be moved back to `AM_CPPCHECKFLAG`s not to alter the code, although it makes it harder to maintain? You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/1346
-- Commit Summary --
* cppcheck: Enable parallelism to match Make call * cppcheck: Properly define GETTEXT_PACKAGE globally * cppcheck: Undefine GEANY_PRIVATE * cppcheck: Use cppcheck's own gtk support * cppcheck: Define the common plugin defines to their actual values * ci: Enable exhaustive cppcheck checking * cppcheck: Enable inline suppression hints * addons: Fix memory leak detected by cppcheck * geanyprj: Fix memory leak detected by cppcheck * workbench: Fix memory leak detected by cppcheck * geniuspaste: Fix memory leak detected by cppcheck * markdown: Slightly rewrite two tests for cppcheck's sake * cppcheck: Add inline suppressions for false-positives * Remove obsolete cppcheck suppressions * treebrowser: Move cppcheck suppressions inline
-- File Changes --
M .github/workflows/build.yml (2) M addons/src/ao_tasks.c (11) M build/cppcheck-geany-plugins.cfg (4) M build/cppcheck.mk (6) M geanyctags/src/geanyctags.c (2) M geanygendoc/src/ggd-options.c (1) M geanyprj/src/Makefile.am (3) M geanyprj/src/utils.c (3) M geniuspaste/src/geniuspaste.c (5) M git-changebar/src/gcb-plugin.c (1) M markdown/src/conf.c (4) M scope/src/Makefile.am (2) M scope/src/debug.c (4) M treebrowser/src/Makefile.am (3) M treebrowser/src/treebrowser.c (2) M vimode/src/cmd-runner.c (2) M workbench/src/sidebar.c (6) M workbench/src/wb_project.c (4) M workbench/src/workbench.c (4)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/1346.patch https://github.com/geany/geany-plugins/pull/1346.diff
@b4n pushed 10 commits.
3d1a5aa72b4e3905e0da108ec1c3a5033bc7961c ci: Enable exhaustive cppcheck checking e3f2816ceb5ec08bd16597c045b130c3d18e43ea cppcheck: Enable inline suppression hints 98fc4fb79834e19bca025626f3d6c53aed3038c2 addons: Fix memory leak detected by cppcheck 1e5d1581a296404276af7a4326d2cf48e8ec9d07 geanyprj: Fix memory leak detected by cppcheck 60a0cce2c242ffff1132006af3a0c658e2b91499 workbench: Fix memory leak detected by cppcheck bdf27a12e48124975a735814518f8ba2eba11ee5 geniuspaste: Fix memory leak detected by cppcheck 697ece0ee4949582c3ece8f0c368357d04532181 markdown: Slightly rewrite two tests for cppcheck's sake 8ca610c273b8780cd9083b0ab7cc41e4fdfb6446 cppcheck: Add inline suppressions for false-positives 0ac49b67b38fb82da9751032620249da526229e2 Remove obsolete cppcheck suppressions 119676eabff36ec59f1bbcac5889a0e1cf66cc13 treebrowser: Move cppcheck suppressions inline
21 minutes might be a tad long…
2 things we could to: * Use a version that doesn't require the exhaustive checking (@eht16 why do we use latest version in the first place? I see 1562ca07500505ab396fe99ce19cb211d1c2c8fa but no rationale… and if it's that the version in Ubuntu 20.04 is too old for something, maybe using 22.04 would be enough?) * we could avoid running cppcheck in the distcheck phase, I'm not sure it has any value… does it?
@b4n pushed 1 commit.
a34f3a35f92598242888982014e3d8b36c763b4f ci: Disable cppcheck on distcheck
- Use a version that doesn't require the exhaustive checking (@eht16 why do we use latest version in the first place? I see [1562ca0](https://github.com/geany/geany-plugins/commit/1562ca07500505ab396fe99ce19cb2...) but no rationale… and if it's that the version in Ubuntu 20.04 is too old for something, maybe using 22.04 would be enough?)
OK, I see #1310 now. Well, maybe we could either be OK with this, use a version that doesn't require this (and hope for a fix), or see if playing with other options has a significant impact without drastic check quality reduction (that last option I'm no hopeful about, because I already did play with it a bit).
- we could avoid running cppcheck in the distcheck phase, I'm not sure it has any value… does it?
I tried disabling this in the last commit above, we'll see what it changes -- but I can still revert that if you think it's a bad idea.
not using --library=gtk
See #1197 for why its needed
Anyway, what do you guys think? Is it good? Are there too many false-positives? Should the suppressions be moved back to AM_CPPCHECKFLAGs not to alter the code, although it makes it harder to maintain?
... should we switch to C++ where its all built into the compiler and doesn't need another tool to apply spurious checks. I'm serious, remember GCC has switched to C++ even though its a C codebase.
I'm not sure it has any value… does it?
Oh how tempting ..... nah, I'll be nice. :stuck_out_tongue_winking_eye:
My 2c is that most plugins are so poorly maintained that if you add cppchecking that actually checks much you will be the one making the patches. You are just adding to your workload.
not using --library=gtk
See #1197 for why its needed
I'm on my phone so can't ask git for details, but l just added it here, so either I bluntly missed it somehow, or it got removed in the meantime, which would be concerning for re-adding it here…
... should we switch to C++ where its all built into the compiler and doesn't need another tool to apply spurious checks. I'm serious,
That's just the C++ grumpy old guy talking, because it's simply not true 😉 Sure C++ has more memory management goodies, but you have to use them in the first place, building the current codebase with a C++ compiler won't magically fix things -- well, it likely won't build without some patching, so in a sense it would avoid all issues… 😂
I'm not sure it has any value… does it?
Oh how tempting ..... nah, I'll be nice. 😜
That's clearly quote misuse… but I laughed, so I give it a pass
My 2c is that most plugins are so poorly maintained that if you add cppchecking that actually checks much, you will be the one making the patches. You are just adding to your workload.
Hopefully it won't find many more issues in the existing code, but could on new one. Though, it's like with all those imperfect tools, the user has to be able to tell whether a warning is meaningful or not, and how to handle it.
or it got removed in the meantime
Or it got removed in the version of #1197 that was eventually merged. Not sure but I think when @eht16 upped the version of cppcheck in CI the `--library=gtk` got built in or something, so it wasn't included in what got merged. Might mean it won't work for older versions of `cppcheck` though.
well, it likely won't build without some patching
Correct, and that fixes the issues. :-) But for sure using a C++ compiler won't fix malloc/free code, but it will allow to progressively move away from it.
Hopefully it won't find many more issues in the existing code
Except for the inevitable improvements (and glitches) with new versions of cppcheck (like above).
Ooops, #1197 is the PR that claims to add `library=gtk` but doesn't, #1196 is the issue that shows the error, off by one, /me needs to be cppchecked :grin:
Anyway, what do you guys think? Is it good? Are there too many false-positives?
Should the suppressions be moved back to `AM_CPPCHECKFLAG`s not to alter the code, although it makes it harder to maintain?
I'm fine with having the suppressions inline. I don't remember if it was me starting to put them into `AM_CPPCHECKFLAG` or suggested by someone. In any way, usually in Python code, I put them inline.
- Use a version that doesn't require the exhaustive checking (@eht16 why do we use latest version in the first place? I see [1562ca0](https://github.com/geany/geany-plugins/commit/1562ca07500505ab396fe99ce19cb2...) but no rationale… and if it's that the version in Ubuntu 20.04 is too old for something, maybe using 22.04 would be enough?)
In the CI we used the cppcheck version available in the Ubuntu release the runner is using. This version tends to be a bit outdated. The nightly builds for Debian Sid use rather new versions (by the design of Sid) and those caused various new findings or other problems which caused the nightly builds to fail. This probably bothers only me but it does. And so I try to fix or workaround the issues or find a hack to make cppcheck happy. This is how #1197, #1310 and maybe more cppcheck related changes emerged.
In general I think for static code checkers it is fine to use recent versions and not rely on versions provided by the distribution to benefit from improved and new checks more quickly than the CI runner images are updated.
- we could avoid running cppcheck in the distcheck phase, I'm not sure it has any value… does it?
Good idea, I don't think it gives any value on distcheck.
Or it got removed in the version of #1197 that was eventually merged. Not sure but I think when @eht16 upped the version of cppcheck in CI the `--library=gtk` got built in or something, so it wasn't included in what got merged. Might mean it won't work for older versions of `cppcheck` though.
I don't mind at all if we use `--library=gtk` or not. AFAIR this was one way to make recent cppcheck versions happy. If there is a better alternative, this is fine.
So, after all, whatever makes the CI, the builds on Debian Sid and @b4n happy, is fine for me :D.
Thanks for the replies 😊 So unless I made a huge mistake (maybe cppcheck my code fixes 😉), I suggest merging this and seeing where it leads us — at the very least it makes the checks green again, and it did reveal a few leaks. It finished in 10 minutes which is not so bad I'd say. If it's a problem, maybe we could see if there's a cppcheck GH action or so to parallelize it and some goodies maybe.
And if some code is really too poorly maintained and cppcheck starts showing it, maybe the sane approach is getting rid of it rather than distributing a time bomb.
(just updated d0192a3621e2505f2fcf6bc1c390278b2396749e to use a more precise cppcheck suppression)
Merged #1346 into master.
As nobody seemed horrified by this, I merged it so we can get CI working again. If there's any issue with this, we can always fix or revert.
github-comments@lists.geany.org