Enhance cppcheck usage, fix CI and fix reported errors

[b4n]

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 😕

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?

[b4n]

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 1562ca0 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]

• Use a version that doesn't require the exhaustive checking (@eht16 why do we use latest version in the first place? I see 1562ca0 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.

[elextr]

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. 😜

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.

[b4n]

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.

[elextr]

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).

[elextr]

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 😁

[eht16]

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?

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 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.

[b4n]

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.

[b4n]

(just updated d0192a3 to use a more precise cppcheck suppression)

[b4n]

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.