cppcheck: use file list and add -j option with available processors

[mdeweerd]

Why

In CI, I enabled cppcheck caching by adding the --cppcheck-build-dir option, but that did not work as every file was analysed in a separate cppcheck call.

The changes avoids splitting the analysis in multiple batches, and has all the files analysed in a single cppcheck call (which itself creates multiple subprocesses).

Summary of the changes

• Use --file-list option to pass multiple files to cppcheck in a single invocation
• Automatically add -j option with number of available processors for parallel analysis
• Add --cppcheck-build-dir when using -j > 1 to enable unusedFunction check compatibility
• Always suppress unusedFunction warnings (now safe due to --cppcheck-build-dir handling)
• Use truly unique temporary directories with tempfile.mkdtemp()
• Properly clean up temporary directories with shutil.rmtree() even on interruption

Changes

• Modified hooks/cppcheck.py to use --file-list option instead of running cppcheck once per file
• Added automatic -j N option with CPU count (only on multi-core systems)
• Added automatic --cppcheck-build-dir creation when using -j > 1
• Simplified suppression logic: always add --suppress=unusedFunction (previously conditional)
• Added proper cleanup of temporary build directories using shutil.rmtree()
• Updated test filters in tests/test_hooks.py to handle cppcheck warning messages

Extra Info

--suppress=unusedFunction is no longer needed

As we add the --cppcheck-build-dir when missing if -j is > 1, there is no longer an issue with the analysis.

Verification

• All existing tests pass - except on macos which is unrelated to these changes
• New behavior properly handles parallel analysis
• Temporary directories are cleaned up even on interruption
• No warning messages about -j and unusedFunction conflicts
• Test on actual project.

[pocc]

Hi mdewweerd,

Thanks for writing this up. The core idea of a single cppcheck invocation instead of one-per-file is a genuine improvement and I appreciate the thorough writeup. A few things are blocking merge for me, in roughly descending order of importance. I've generated this with my SOTA model of choice, but feel free to push back if it's missing the mark. The unusedFunction failing on a partial set of files due to how pre-commit stages files makes suppressing this flag seem like the correct behavior.

I ran the PR head (346dc7e) against cppcheck 2.19 and have a few blocking concerns. Everything below is reproducible with one self-contained script RIGHT BELOW THIS LINE (click on the ▶).

demo_review.py — self-contained reproduction (run from the PR repo root)

#!/usr/bin/env python3
"""Self-contained reproduction for PR #73 review.

Run from the root of the PR checkout (so `import hooks` resolves):

    python3 demo_review.py

Requires: cppcheck on PATH. Creates its own temp C files; cleans them up.
"""
import glob
import os
import shutil
import subprocess
import sys
import tempfile

from hooks.cppcheck import CppcheckCmd

TMP = tempfile.gettempdir()


def make_sources():
    """helper.c defines add(); main.c is its only caller."""
    d = tempfile.mkdtemp(prefix="demo_src_")
    helper = os.path.join(d, "helper.c")
    main = os.path.join(d, "main.c")
    with open(helper, "w") as f:
        f.write("int add(int a, int b) {\n    return a + b;\n}\n")
    with open(main, "w") as f:
        f.write("int add(int a, int b);\nint main(void) {\n    return add(2, 3);\n}\n")
    return d, helper, main


def hook(*files):
    """Invoke the hook exactly as pre-commit would; return exit code."""
    r = subprocess.run([sys.executable, "-m", "hooks.cppcheck", "cppcheck-hook", *files])
    return r.returncode


def cppcheck_builddirs():
    return sorted(glob.glob(os.path.join(TMP, "cppcheck_*")))


def demo1(helper, main):
    print("=" * 60)
    print("#1  unusedFunction false positive on a partial commit")
    print("=" * 60)
    print("\n-- stage ONLY helper.c (add() is used in unstaged main.c):")
    print(f"[exit code: {hook(helper)}]")
    print("\n-- stage BOTH files (add() now visibly used):")
    print(f"[exit code: {hook(helper, main)}]")


def demo2(helper):
    print("\n" + "=" * 60)
    print("#2  auto --cppcheck-build-dir gives no cross-run caching")
    print("=" * 60)
    for d in cppcheck_builddirs():
        shutil.rmtree(d, ignore_errors=True)
    dirs = []
    for i in (1, 2):
        c = CppcheckCmd(["cppcheck-hook", helper])
        bd = next((a.split("=", 1)[1] for a in c.args if a.startswith("--cppcheck-build-dir")), None)
        dirs.append(bd)
        try:
            c.run()
        except SystemExit:
            pass
        print(f"run {i}: build dir = {bd}   exists after run()? {os.path.isdir(bd) if bd else 'n/a'}")
    print(f"run1 and run2 used the SAME dir (cache reuse)? {dirs[0] == dirs[1]}")
    print(f"persistent cppcheck_* dirs left behind: {len(cppcheck_builddirs())}")


def demo3(helper):
    print("\n" + "=" * 60)
    print("#3  mkdtemp() in __init__ leaks when run() is never called")
    print("=" * 60)
    before = set(cppcheck_builddirs())
    cmd = CppcheckCmd(["cppcheck-hook", helper])  # construct only; no run()
    leaked = sorted(set(cppcheck_builddirs()) - before)
    print(f"build dirs created by __init__ alone (no run()): {len(leaked)}")
    for d in leaked:
        print(f"   leaked -> {d}   exists: {os.path.isdir(d)}")
    print(f"--suppress=unusedFunction in args? {'--suppress=unusedFunction' in cmd.args}")
    print("=> original assertion `assert \"--suppress=unusedFunction\" in cmd.args` would:",
          "PASS" if "--suppress=unusedFunction" in cmd.args else "FAIL")
    for d in leaked:
        shutil.rmtree(d, ignore_errors=True)


def main():
    d, helper, mainc = make_sources()
    try:
        demo1(helper, mainc)
        demo2(helper)
        demo3(helper)
    finally:
        shutil.rmtree(d, ignore_errors=True)
        for x in cppcheck_builddirs():
            shutil.rmtree(x, ignore_errors=True)


if __name__ == "__main__":
    main()

How to reproduce

git fetch origin pull/73/head:pr-73 && git checkout pr-73
# save demo_review.py (below) at the repo root, then:
python3 demo_review.py          # requires cppcheck on PATH

1. Dropping --suppress=unusedFunction → false positives on partial commits (blocking)

pre-commit passes only the staged files to the hook. unusedFunction flags a function as unused when it sees no caller — but the caller may live in a file that isn't staged. The script writes:

// helper.c  — defines add()
int add(int a, int b) { return a + b; }

// main.c  — the only caller of add()
int add(int a, int b);
int main(void) { return add(2, 3); }

Actual output:

#1  unusedFunction false positive on a partial commit
-- stage ONLY helper.c (add() is used in unstaged main.c):
helper.c:1:5: style: The function 'add' is never used. [unusedFunction]
[exit code: 1]        <-- commit blocked

-- stage BOTH files (add() now visibly used):
[exit code: 0]

The failure depends entirely on which files happen to be staged — the defining trait of a false positive, and exactly why the suppression was added in #30. Committing an edit to helper.c alone would fail for every user. I'd keep --suppress=unusedFunction.

2. The auto --cppcheck-build-dir provides no caching (the PR's stated goal)

The auto path mkdtemp()s a dir and rmtree()s it in finally, so it's unique per run and never survives:

#2  auto --cppcheck-build-dir gives no cross-run caching
run 1: build dir = /tmp/cppcheck_zf4l2ann   exists after run()? False
run 2: build dir = /tmp/cppcheck_y_6pgf_l   exists after run()? False
run1 and run2 used the SAME dir (cache reuse)? False
persistent cppcheck_* dirs left behind: 0

Caching only helps when the dir persists and is reused, so this adds -j/build-dir complexity without delivering it. (Caching already worked when a user passed their own --cppcheck-build-dir; that path is untouched.) I'd drop the auto build-dir.

3. mkdtemp() in the constructor leaks temp dirs

add_j_option() runs from __init__, so merely constructing the command writes a dir to disk — even when run() is never called (cleanup only happens in run()/main()'s finally):

#3  mkdtemp() in __init__ leaks when run() is never called
build dirs created by __init__ alone (no run()): 1
   leaked -> /tmp/cppcheck__z6hgrd7   exists: True
--suppress=unusedFunction in args? False
=> original assertion `assert "--suppress=unusedFunction" in cmd.args` would: FAIL

The PR's own new tests/test_cppcheck.py constructs instances without running them, so each test leaks a dir. I'd move temp-dir creation into run().

4. The regression test was disabled, not updated

The False above is why tests/test_logic_regression.py had to change — but the guard was commented out rather than replaced:

- assert "--suppress=unusedFunction" in cmd.args
+ # assert "--suppress=unusedFunction" in cmd.args   # Not needed

Likewise tests/test_hooks.py adds regex filters that strip cppcheck's own -j/unusedFunction warnings before comparison, hiding version-dependent behavior instead of resolving it.

5. Simpler alternative for the single-invocation part

If the goal is just one cppcheck call, --file-list=- over stdin does it with no temp file, no delete=False, and no cleanup path to get wrong:

def run(self):
    self.run_command(self.args + ["--file-list=-"],
                     input_data="\n".join(self.files).encode())
    self.exit_on_error()

6. Could you squash?

The series has commits that reverse each other's decisions (add suppression → remove → re-add build-dir → "always suppress" → "stop adding suppression") and a few Closes # with no issue number, which makes the net change hard to review.

Happy to take the -j parallelism on top of a stdin-based single invocation with unusedFunction still suppressed — that keeps the win without the regression.

---

If you'd like to keep just the -j parallelism on top of a stdin-based single invocation (with unusedFunction still suppressed), I'd happily take that. The minor HTTP => HTTPS link corrections are also welcome.

[mdeweerd]

1. Dropping --suppress=unusedFunction → false positives on partial commits (blocking)

Actually '--enable=all' was the culprit. Suppressing the notice did not prevent the warning regarding the unusedFunction checks not being compatible.

-> I updated the options to 'enable' which allows suppressing all the rest. If a user sets '--enable=all', the warning will be visible and require setting '-j1' to maintain unusedFunction.

2. The auto --cppcheck-build-dir provides no caching (the PR's stated goal)

--cppcheck-build-dir can provide caching when the path is defined in the .pre-commit-config.yaml .

I set the cppcheck build path to .cache/cppcheck. Locally that path's contents remains in place, in CI that path is cached using the cache action.

When the user does not provide it, it's set to a temporary directory as we need to define it for the parallel run (otherwise, cppcheck itself is not running properly) and cleaned up.

3. mkdtemp() in the constructor leaks temp dirs

Updated.

4. The regression test was disabled, not updated

The False above is why tests/test_logic_regression.py had to change — but the guard was commented out rather than replaced:

The logic behind this is that the suppression of the unusedFunction was no longer forced.

Now it's no longer needed.

5. Simpler alternative for the single-invocation part

Ok, to pass the file list over stdin, implemented.

6. Could you squash?

Yes, I can. Done.

Summary

• Anything with regards to 'unusedFunction' suppressed by not using '--enable=all' causing all the trouble.
• Parallellism automatically enabled.
• Test cases added and tests with regards to unusedFunction removed.

IMHO, the non-passing tests on windows and macos are not related to this update.

[pocc]

Thanks for the revision. This addresses most of the review. #1 is fixed (dropping unusedFunction from --enable means cppcheck helper.c alone no longer fails), #2 caching now works when a persistent --cppcheck-build-dir is configured, #4 the test masking is gone, and #5/#6 (stdin --file-list=-, squash) are done. One issue remains: the temp-dir cleanup (#3) is still broken, and now leaks on every run rather than only on construct-without-run.

There are two mkdtemp() calls writing to the same field. set_j_value() (called in __init__) creates a dir but never passes it to cppcheck:

# cppcheck.py:77-78  (set_j_value)
self.cppcheck_build_dir = tempfile.mkdtemp(prefix="cppcheck_")   # dir A
self.args.extend(["-j", str(num_cpus)])                          # note: --cppcheck-build-dir is NOT added

Then run() calls set_create_build_dir_if_needed(), which sees no build-dir arg, creates a second dir, and overwrites the field:

# cppcheck.py:91-95
has_build_dir = any(arg.startswith("--cppcheck-build-dir") for arg in self.args)  # False — A was never added
if not has_build_dir:
    self.cppcheck_build_dir = tempfile.mkdtemp(prefix="cppcheck_")   # dir B, reference to A is now lost
    self.add_if_missing([f"--cppcheck-build-dir={self.cppcheck_build_dir}"])

cleanup_build_dir() (cppcheck.py:110) only removes self.cppcheck_build_dir, which is now B. Dir A is orphaned every run, with no reference to clean it up.

Minimal repro — invoke the hook exactly as pre-commit does and count what's left behind:

T=$(python3 -c 'import tempfile;print(tempfile.gettempdir())')
before=$(ls -d "$T"/cppcheck_* 2>/dev/null | wc -l)
printf 'int add(int a,int b){return a+b;}\n' > "$T/helper.c"
for i in 1 2 3; do python3 -m hooks.cppcheck cppcheck-hook "$T/helper.c" >/dev/null 2>&1; done
echo "leaked: $(( $(ls -d "$T"/cppcheck_* 2>/dev/null | wc -l) - before ))"

I see leaked: 3, which would seem to indicate that this is still an issue.

Three commits, three orphaned temp dirs will create one per invocation, indefinitely. This isn't that big of a deal because these will be cleaned on restart, but it's cruft we don't need to create. The fix is small: drop the mkdtemp() in set_j_value() (it only needs to add -j N) and let run() own the single build dir.

[mdeweerd]

I removed the mkdtemp from set_j_option and reinforced checks prior to creating the directory.