Add cppcheck action

[Neverlord]

Relates #249.

[awelzel]

@Neverlord - have you considered doing this via the pre-commit setup?

There's the following, somewhat stale repo, but we could always fork if we need to:
https://github.com/pocc/pre-commit-hooks

@bbannier - any thoughts?

(Mostly because maybe that'd allow to just use it for zeek/zeek, too)

[bbannier]

IMO pre-commit is preferable as it makes running these checks locally much easier. As for the hook, since some version of cppcheck is available on pypi the following setup also works:

repos:
- repo: local
  hooks:
  - id: cppcheck
    name: cppcheck
    language: python
    entry: cppcheck
    types_or:
      - "c"
      - "c++"
    additional_dependencies:
      - cppcheck

Note that this currently causes no diagnostics to be emitted.

That said, in my experience cppcheck provides not much on top of clang-tidy, but is much harder to set up, e.g., the change to an explicit ctr in this PR could just as well be implemented with clang-tidy's google-explicit-constructor, and I'd just not implement cppcheck here at all.

[bbannier]

An alternative to hardcoding flags for CI would be to run cppcheck as part of CMake.

[Neverlord]

Thanks for the early feedback here!

have you considered doing this via the pre-commit setup?

Didn't cross my mind. To me, a pre-commit hook is for quick checks like formatting, spell checking, etc. that take a couple seconds to run. Using a pre-commit hook to run a static analyzer doesn't fall into that category in my mental model. I can see the argument for convenience, but cppcheck is basically available anywhere. We already have ci/analyze.sh that I can run locally to run clang-tidy if I really want to. So maybe we could just offer a similar thing for cppcheck or extend the existing script with a CLI switch?

At least for me, running a static analyzer isn't part of the regular work flow. I see it as a quality gate on a PR and if the pipeline complains, I'll look at the output/artifact fix the issue and push again. Usually it's not something I'd need to reproduce locally.

But that's just me saying I wouldn't use it and thus didn't consider it. If there are other work flows on the team that integrate tools like this - just tell me what I need to do. 🙂

An alternative to hardcoding flags ...

The usual way to run cppcheck is the same as for clang-tidy: using the compile database. I didn't get that to work, because unfortunately cppcheck (at least in the versions I've tried for CI) doesn't understand include paths via -isystem (that's what CMake will pass to the compiler for includes tagged as SYSTEM). I don't know if CMake would do something different when setting CXX_CPPCHECK, but at least from the documentation I don't see a way to generate the HTML report that we generate as artifact for CI? Am I just missing something obvious?

FWIW, cppcheck and clang-tidy definitely do have overlap but I still think running both has value since cppcheck has it's own independent implementation. Of course, if the consensus is that we won't integrate both than please just close the PR and I'll maybe cherry-pick some of the changes later.

[awelzel]

Using a pre-commit hook to run a static analyzer doesn't fall into that category in my mental model.

This is fair and if cppcheck isn't super fast, pre-commit isn't the right place. Thanks.

if I really want to. So maybe we could just offer a similar thing for cppcheck or extend the existing script with a CLI switch?

Yeah, that sound good. I primarily didn't like the cppcheck invocation with all the flags in a file somewhere within .github - ci/cppcheck.sh that is then invoked from the GitHub job works nicely IMO. Thanks!