Gate every push on cppcheck: 103 findings, and two files cppcheck cannot parse at all #83

New Issue

Open

opened 2026-08-05 15:53:56 -04:00 by tachikoma · 1 comment

tachikoma commented 2026-08-05 15:53:56 -04:00

Collaborator

Copy Link

Add a cppcheck static-analysis gate to this repository: run on every push, fail the
pipeline on any finding, and run again in the pre-push hook so it fails on the workstation
first. This is one of four -- akbasic, libakerror, libakstdlib and libakgl all get the same
gate, built to the same plan, and this issue is the libakgl half.

Do libakerror and libakstdlib first. They are 6 and 15 findings and they are where the
shape of scripts/cppcheck.sh and the static_analysis job gets settled; this repository
copies it. libakgl is 103 findings and, with akbasic, the larger cleanup of the four.

Measured, before any change

cppcheck 2.19.0, --std=c99 --platform=unix64 --enable=style.

Over src include:

severity	count
error	1
warning	0
style	90

Over examples tools util: 1 error, 11 style.

103 findings in total. The style bulk is two ids and nothing else worth naming:
unreadVariable 40 and variableScope 35, then constParameterPointer 8 and
constVariablePointer 3. Concentrated rather than spread: src/collision.c 16,
src/tilemap.c 14, src/draw.c 14, src/controller.c 11, src/collision_grid.c 11.

Both "errors" are cppcheck failing to parse, not defects:

src/ui.c:877:39: error: Syntax Error: AST broken, 'look' doesn't have a parent [internalAstError]
examples/uidemo/uidemo.c:374:3: error: AST broken, ternary operator missing operand(s) [internalAstError]

These are the two that decide whether this gate is viable here, so take them first. An
internalAstError means cppcheck gave up on part of a translation unit -- so whatever else
is in that file went unchecked, silently. Both sites are UI code, and Clay's declarative
macros are the obvious suspect: they build layout blocks out of for-loop and
compound-literal tricks that a C parser which is not a compiler can legitimately choke on.

Two of the three ways out are acceptable and one is not. Acceptable: find the construct and
establish whether cppcheck can be helped over it (-D/-U on the offending macro, or
--max-configs), or accept the limit and exclude the specific file with a comment saying
what is not being checked and why. Not acceptable: leave the id suppressed globally, which
turns every future parse failure anywhere in the tree into silence.

The 35 variableScope and 40 unreadVariable are mechanical, but 40 assigned-and-never-read
variables in a graphics library is worth a skim before a mass edit -- some fraction of those
are a result computed and then not checked.

Scope for this repository

SCOPE=( src include examples tools util )

deps/ is excluded by the scope and that is load-bearing here: this tree vendors SDL,
SDL_image, SDL_mixer, SDL_ttf, jansson, semver, libccd, clay and tg. A bare cppcheck .
would check all of them -- thousands of findings in code this repository cannot fix, and
minutes of runtime. tests/ (29 files) is excluded per the plan below.

One local wrinkle: include/akgl/SDL_GameControllerDB.h is generated, and
scripts/hooks/pre-commit already excludes it from the reindent scope for that reason. If
it produces findings, it is excluded here on the same grounds -- fix generators, never
generated output.

Work

• Resolve the two internalAstError sites first; they decide how much of the tree this
  gate can actually see. No global suppression of that id.
• Skim the 40 unreadVariable for results that were computed and never checked. Any
  that turn out to be real defects get their own issue and a link back here.
• Fix or inline-suppress the remaining findings. Every suppression carries its reason on
  the line above it.
• Add scripts/cppcheck.sh with the scope above.
• Add the static_analysis job to .gitea/workflows/ci.yaml. It needs no submodules
  and none of the X11/freetype dependency list the other jobs carry: cppcheck does not
  link, and the scope is this repository's own C.
• Prove the gate red once on a throwaway branch, then delete the branch.
• Wire it into the pre-push hook. This repository has scripts/hooks/ with a
  pre-commit hook but no pre-push hook -- that is #82, and this box waits
  on it.
• Record the tests/ exclusion in TODO.md with its consequence.

---

The plan (identical in all four repositories)

The gate

cppcheck <scope> \
    --std=c99 \
    --platform=unix64 \
    --enable=style \
    --inline-suppr \
    --error-exitcode=1 \
    --quiet

Four deviations from the sketch, each with a measurement behind it:

1. An explicit scope, not .. cppcheck . descends into deps/ and into whatever
   build*/ happens to be lying around. Measured in libakstdlib: cppcheck . -i tests
   reports 30 findings, 24 of them inside deps/libakerror -- the same six defects this
   plan already gates in libakerror's own pipeline, reported a second time from a
   repository that cannot fix them. akbasic and libakgl vendor SDL, SDL_image, SDL_mixer,
   SDL_ttf, jansson, libccd and clay, which is thousands of findings and minutes of
   runtime for code nobody here owns. Build directories are the other half: the mutation
   harness copies the whole tree into one, and the pre-push hook runs on a workstation
   where two or three of them exist.

2. --error-exitcode=1. cppcheck exits 0 when it has findings. Verified on 2.19.0:
   exit 0 with six findings reported, exit 1 with the flag, exit 0 on a clean tree. Without
   this flag the job is green no matter what it prints.

3. --inline-suppr, and no baseline file. Every finding is either fixed or carries a
   // cppcheck-suppress <id> on the line above it with the reason in a comment. A
   suppression list in a separate file is a list nobody reads; a suppression at the site is
   a documented, tracked defect, which is what the engineering rules ask for. There is no
   "current findings" baseline: the tree goes clean before the gate turns on.

4. -i tests, as sketched -- deferred, not decided against. Tests are where a leak is
   most likely to hide, and this excludes them. That is a named deferral for TODO.md, not a
   judgement that test code does not matter. (-i 'tests/*' and -i tests both work on
   2.19.0; an explicit scope makes the question moot.)

Where it lives

scripts/cppcheck.sh -- one definition that CI and the pre-push hook both run, so a push
that would fail CI fails on the workstation first. Same shape as the existing
scripts/coverage.py, scripts/memcheck.sh and scripts/mutation_test.py.

#!/usr/bin/env bash
#
# Static analysis gate. Run by .gitea/workflows/ci.yaml and by the pre-push hook,
# so both check the same thing.

set -u

if ! command -v cppcheck > /dev/null 2>&1; then
    echo "cppcheck: not installed" >&2
    exit 127
fi

root=$(git rev-parse --show-toplevel) || exit 1
cd "$root" || exit 1

exec cppcheck "${SCOPE[@]}" \
    --std=c99 \
    --platform=unix64 \
    --enable=style \
    --inline-suppr \
    --error-exitcode=1 \
    --quiet \
    "$@"

SCOPE is the repository's own C and nothing else -- named per repository below.

CI

A new static_analysis job:

  static_analysis:
    runs-on: ubuntu-latest
    steps:
      - name: Check out repository code
        uses: actions/checkout@v4
        # No submodules on purpose: the scope is this repository's own C, and the
        # dependencies gate themselves in their own pipelines.
      - name: dependencies
        run: |
          sudo apt-get update -y
          sudo apt-get install -y cppcheck
      # Findings move with the version. Record which cppcheck gated this run, so a
      # red job that nobody's commit caused can be identified as such in one look.
      - run: cppcheck --version
      - name: static analysis
        run: scripts/cppcheck.sh
      - run: echo "🍏 This job's status is ${{ job.status }}."

The check itself is 2 s (libakgl) to 6 s (akbasic) measured on this workstation; the apt
install dominates the job.

Version drift is a real cost and is accepted rather than engineered around: a newer
cppcheck finds new things, and the gate goes red on a push that did not cause it. That is
fix-forward. Printing the version is what makes it diagnosable. Do not pin an old cppcheck
to keep the tree quiet.

pre-push

First step in the hook -- it is the cheapest gate there by an order of magnitude. Skipped
with a warning when cppcheck is not installed, matching the doxygen precedent in
libakstdlib's hook: a hook that fails closed on a missing optional tool only teaches
everyone to pass --no-verify. CI is the hard gate.

Order

1. libakerror (6 findings) -- smallest tree, one source file. Proves the script and the
   job shape; the other three copy it.
2. libakstdlib (15, one of them a real error) -- already has .githooks/pre-push.
3. akbasic (102) and libakgl (103) in parallel -- both need a pre-push hook first.

Acceptance, per repository

• scripts/cppcheck.sh exits 0 on a clean checkout with no suppressions beyond those whose
  reason is written at the site.
• The static_analysis job exists and has been proven red once, by pushing a deliberate
  finding on a throwaway branch and watching the pipeline fail.
• The pre-push hook runs it -- or the hook bug is filed, linked here, and this repository's
  wiring waits on it.
• Every remaining finding is either fixed or inline-suppressed with a reason.
• The -i tests deferral is recorded in TODO.md with its consequence.

Measured with cppcheck 2.19.0 on 2026-08-05.

Add a `cppcheck` static-analysis gate to this repository: run on every push, fail the pipeline on any finding, and run again in the pre-push hook so it fails on the workstation first. This is one of four -- akbasic, libakerror, libakstdlib and libakgl all get the same gate, built to the same plan, and this issue is the libakgl half. Do libakerror and libakstdlib first. They are 6 and 15 findings and they are where the shape of `scripts/cppcheck.sh` and the `static_analysis` job gets settled; this repository copies it. libakgl is 103 findings and, with akbasic, the larger cleanup of the four. ## Measured, before any change cppcheck 2.19.0, `--std=c99 --platform=unix64 --enable=style`. Over `src include`: | severity | count | | --- | --- | | error | 1 | | warning | 0 | | style | 90 | Over `examples tools util`: 1 error, 11 style. **103 findings in total.** The style bulk is two ids and nothing else worth naming: `unreadVariable` 40 and `variableScope` 35, then `constParameterPointer` 8 and `constVariablePointer` 3. Concentrated rather than spread: `src/collision.c` 16, `src/tilemap.c` 14, `src/draw.c` 14, `src/controller.c` 11, `src/collision_grid.c` 11. Both "errors" are cppcheck failing to parse, not defects: ``` src/ui.c:877:39: error: Syntax Error: AST broken, 'look' doesn't have a parent [internalAstError] examples/uidemo/uidemo.c:374:3: error: AST broken, ternary operator missing operand(s) [internalAstError] ``` These are the two that decide whether this gate is viable here, so take them first. An `internalAstError` means cppcheck gave up on part of a translation unit -- so whatever else is in that file went unchecked, silently. Both sites are UI code, and Clay's declarative macros are the obvious suspect: they build layout blocks out of `for`-loop and compound-literal tricks that a C parser which is not a compiler can legitimately choke on. Two of the three ways out are acceptable and one is not. Acceptable: find the construct and establish whether cppcheck can be helped over it (`-D`/`-U` on the offending macro, or `--max-configs`), or accept the limit and exclude the specific file with a comment saying what is not being checked and why. Not acceptable: leave the id suppressed globally, which turns every future parse failure anywhere in the tree into silence. The 35 `variableScope` and 40 `unreadVariable` are mechanical, but 40 assigned-and-never-read variables in a graphics library is worth a skim before a mass edit -- some fraction of those are a result computed and then not checked. ## Scope for this repository ``` SCOPE=( src include examples tools util ) ``` `deps/` is excluded by the scope and that is load-bearing here: this tree vendors SDL, SDL_image, SDL_mixer, SDL_ttf, jansson, semver, libccd, clay and tg. A bare `cppcheck .` would check all of them -- thousands of findings in code this repository cannot fix, and minutes of runtime. `tests/` (29 files) is excluded per the plan below. One local wrinkle: `include/akgl/SDL_GameControllerDB.h` is generated, and `scripts/hooks/pre-commit` already excludes it from the reindent scope for that reason. If it produces findings, it is excluded here on the same grounds -- fix generators, never generated output. ## Work - [ ] Resolve the two `internalAstError` sites first; they decide how much of the tree this gate can actually see. No global suppression of that id. - [ ] Skim the 40 `unreadVariable` for results that were computed and never checked. Any that turn out to be real defects get their own issue and a link back here. - [ ] Fix or inline-suppress the remaining findings. Every suppression carries its reason on the line above it. - [ ] Add `scripts/cppcheck.sh` with the scope above. - [ ] Add the `static_analysis` job to `.gitea/workflows/ci.yaml`. It needs no submodules and none of the X11/freetype dependency list the other jobs carry: cppcheck does not link, and the scope is this repository's own C. - [ ] Prove the gate red once on a throwaway branch, then delete the branch. - [ ] Wire it into the pre-push hook. **This repository has `scripts/hooks/` with a pre-commit hook but no pre-push hook** -- that is #82, and this box waits on it. - [ ] Record the `tests/` exclusion in TODO.md with its consequence. --- # The plan (identical in all four repositories) ## The gate ```sh cppcheck <scope> \ --std=c99 \ --platform=unix64 \ --enable=style \ --inline-suppr \ --error-exitcode=1 \ --quiet ``` Four deviations from the sketch, each with a measurement behind it: 1. **An explicit scope, not `.`.** `cppcheck .` descends into `deps/` and into whatever `build*/` happens to be lying around. Measured in libakstdlib: `cppcheck . -i tests` reports 30 findings, 24 of them inside `deps/libakerror` -- the same six defects this plan already gates in libakerror's own pipeline, reported a second time from a repository that cannot fix them. akbasic and libakgl vendor SDL, SDL_image, SDL_mixer, SDL_ttf, jansson, libccd and clay, which is thousands of findings and minutes of runtime for code nobody here owns. Build directories are the other half: the mutation harness copies the whole tree into one, and the pre-push hook runs on a workstation where two or three of them exist. 2. **`--error-exitcode=1`.** cppcheck exits 0 when it has findings. Verified on 2.19.0: exit 0 with six findings reported, exit 1 with the flag, exit 0 on a clean tree. Without this flag the job is green no matter what it prints. 3. **`--inline-suppr`, and no baseline file.** Every finding is either fixed or carries a `// cppcheck-suppress <id>` on the line above it with the reason in a comment. A suppression list in a separate file is a list nobody reads; a suppression at the site is a documented, tracked defect, which is what the engineering rules ask for. There is no "current findings" baseline: the tree goes clean before the gate turns on. 4. **`-i tests`, as sketched -- deferred, not decided against.** Tests are where a leak is most likely to hide, and this excludes them. That is a named deferral for TODO.md, not a judgement that test code does not matter. (`-i 'tests/*'` and `-i tests` both work on 2.19.0; an explicit scope makes the question moot.) ## Where it lives `scripts/cppcheck.sh` -- one definition that CI and the pre-push hook both run, so a push that would fail CI fails on the workstation first. Same shape as the existing `scripts/coverage.py`, `scripts/memcheck.sh` and `scripts/mutation_test.py`. ```sh #!/usr/bin/env bash # # Static analysis gate. Run by .gitea/workflows/ci.yaml and by the pre-push hook, # so both check the same thing. set -u if ! command -v cppcheck > /dev/null 2>&1; then echo "cppcheck: not installed" >&2 exit 127 fi root=$(git rev-parse --show-toplevel) || exit 1 cd "$root" || exit 1 exec cppcheck "${SCOPE[@]}" \ --std=c99 \ --platform=unix64 \ --enable=style \ --inline-suppr \ --error-exitcode=1 \ --quiet \ "$@" ``` `SCOPE` is the repository's own C and nothing else -- named per repository below. ## CI A new `static_analysis` job: ```yaml static_analysis: runs-on: ubuntu-latest steps: - name: Check out repository code uses: actions/checkout@v4 # No submodules on purpose: the scope is this repository's own C, and the # dependencies gate themselves in their own pipelines. - name: dependencies run: | sudo apt-get update -y sudo apt-get install -y cppcheck # Findings move with the version. Record which cppcheck gated this run, so a # red job that nobody's commit caused can be identified as such in one look. - run: cppcheck --version - name: static analysis run: scripts/cppcheck.sh - run: echo "🍏 This job's status is ${{ job.status }}." ``` The check itself is 2 s (libakgl) to 6 s (akbasic) measured on this workstation; the apt install dominates the job. Version drift is a real cost and is accepted rather than engineered around: a newer cppcheck finds new things, and the gate goes red on a push that did not cause it. That is fix-forward. Printing the version is what makes it diagnosable. Do not pin an old cppcheck to keep the tree quiet. ## pre-push First step in the hook -- it is the cheapest gate there by an order of magnitude. Skipped with a warning when cppcheck is not installed, matching the doxygen precedent in libakstdlib's hook: a hook that fails closed on a missing optional tool only teaches everyone to pass `--no-verify`. CI is the hard gate. ## Order 1. **libakerror** (6 findings) -- smallest tree, one source file. Proves the script and the job shape; the other three copy it. 2. **libakstdlib** (15, one of them a real error) -- already has `.githooks/pre-push`. 3. **akbasic** (102) and **libakgl** (103) in parallel -- both need a pre-push hook first. ## Acceptance, per repository - `scripts/cppcheck.sh` exits 0 on a clean checkout with no suppressions beyond those whose reason is written at the site. - The `static_analysis` job exists and has been proven red once, by pushing a deliberate finding on a throwaway branch and watching the pipeline fail. - The pre-push hook runs it -- or the hook bug is filed, linked here, and this repository's wiring waits on it. - Every remaining finding is either fixed or inline-suppressed with a reason. - The `-i tests` deferral is recorded in TODO.md with its consequence. Measured with cppcheck 2.19.0 on 2026-08-05.

tachikoma added the hygieneblast-radius:mediumstatus::ready labels 2026-08-05 15:53:56 -04:00

tachikoma referenced this issue from andrew/libakerror 2026-08-05 15:54:04 -04:00

Gate every push on cppcheck: 6 findings, all in src/error.c #33

tachikoma referenced this issue from andrew/libakstdlib 2026-08-05 15:54:05 -04:00

Gate every push on cppcheck: 15 findings, one of them a reported use-after-free in aksl_fclose #45

tachikoma referenced this issue from andrew/akbasic 2026-08-05 15:54:05 -04:00

Gate every push on cppcheck: 102 findings, including a missing return in src/sink_akgl.c that no CI build compiles #48

tachikoma commented 2026-08-05 15:54:06 -04:00

Author

Collaborator

Copy Link

Siblings, same plan:

• akbasic: andrew/akbasic#48 (hook bug: andrew/akbasic#47)
• libakerror: andrew/libakerror#33 (hook bug: andrew/libakerror#32)
• libakstdlib: andrew/libakstdlib#45 (already has .githooks/pre-push)
• libakgl: andrew/libakgl#83 (hook bug: andrew/libakgl#82)

Order: libakerror, then libakstdlib, then akbasic and libakgl in parallel.

Siblings, same plan: - akbasic: andrew/akbasic#48 (hook bug: andrew/akbasic#47) - libakerror: andrew/libakerror#33 (hook bug: andrew/libakerror#32) - libakstdlib: andrew/libakstdlib#45 (already has `.githooks/pre-push`) - libakgl: andrew/libakgl#83 (hook bug: andrew/libakgl#82) Order: libakerror, then libakstdlib, then akbasic and libakgl in parallel.

andrew added a new dependency 2026-08-05 16:08:17 -04:00

#82 scripts/hooks/ has a pre-commit hook but no pre-push hook

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

82

todo-to-issues

clay-ui

coverage-test-plan

feature/physics_bsptree

bugfix/physics

feature/relative_path_loading

No results found.

Labels

Clear labels

abi-break

Cannot land without a soname bump

api-gap

A caller cannot express something

blast-radius:high

Ordering: highest blast radius

blast-radius:low

Ordering: lowest blast radius

blast-radius:medium

Ordering: middling blast radius

defect

Wrong behaviour reachable from the public API

design-decision

Needs a decision written down before code

docs

Documentation, comments, AGENTS.md

hygiene

Style, dead code, consistency

packaging

Build, install, pkg-config, CMake config

performance

Has a measurement behind it

status::blocked

Waiting on something else -- a decision, a dependency, or another issue

status::grooming

Not yet worked out: scope, approach or priority still being settled

status::ready

Understood well enough to start: scope and approach are settled

status::review

A pull request has been generated and this ticket is waiting on review of that PR

test-coverage

A gap in the suites, not a defect

No Label blast-radius:medium hygiene status::ready

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

logikoma tachikoma andrew

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Depends on

#82 scripts/hooks/ has a pre-commit hook but no pre-push hook

andrew/libakgl

Reference: andrew/libakgl#83