Build clean under -Wall, with -Werror in CI only

TODO.md item 37 said the cast sweep buys nothing until the build turns on the
warnings those casts suppress. This is that precondition, and the argument
turned out to be right with evidence: -Wall found three genuine signedness
mismatches in sprite.c, where uint32_t width, height and speed were passed
straight to akgl_get_json_integer_value(..., int *). A cast would have silenced
all three. Fixing them properly also turned up that speed is scaled by a million
into a 32-bit field, so anything past 4294 ms overflowed rather than being held.

The other real finding was ten -Wstringop-truncation warnings, every one a
fixed-width strncpy. Those leave a name field unterminated when the input fills
it -- and those fields are registry keys, handed to strcmp and SDL property
calls that do not stop at the field. Same overread class fixed twice already
this release. All ten now use aksl_strncpy from akstdlib, which always
terminates and never NUL-pads, bounded to size - 1 so an over-long name still
truncates rather than being refused. staticstring.h documented the old strncpy
semantics explicitly and has been rewritten to match.

Two sites in game.c are deliberately left on strncpy: both stage into a
pre-zeroed buffer for the savegame's fixed-width fields, where the NUL padding
is part of the format. Neither is flagged.

sprite.c also had a memcpy of a full 128-byte field from a NUL-terminated
string, which reads past the end of any shorter name. Not flagged by anything;
found while converting its neighbours.

-Werror is an option, default OFF, on only in the cmake_build and performance CI
jobs. libakgl is consumed with add_subdirectory -- akbasic does exactly that --
so a target-level -Werror turns every diagnostic a newer compiler invents into a
broken build for somebody else. The warning set is not even stable across build
types here: an -O2 build reports ten warnings that -O0 and -fsyntax-only do not,
because they need the middle end. The two jobs that set it are one of each.

-Wextra is deliberately not adopted. It adds 22 findings, 17 inherent to the
design: 13 -Wunused-parameter from backend vtables and SDL callbacks that must
match a signature, and 4 -Wimplicit-fallthrough from libakerror's own
PROCESS/HANDLE/HANDLE_GROUP, which fall through by design. Filed upstream as
deps/libakerror TODO item 8; the submodule bump carries it.

deps/semver/semver.c is listed directly in add_library, so the target's PRIVATE
options reach it. Exempted with -w so a future semver update cannot fail this
build -- verified by forcing -Wextra -Werror and watching our files fail while
semver compiled clean.

Verified: RelWithDebInfo, Debug and coverage builds all clean under -Werror;
-Werror actually fails on a planted unused variable; an embedded consumer with
AKGL_WERROR unset still configures and builds. 25/25 pass, memcheck clean,
reindent --check, check_api_surface and check_error_protocol clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Andrew Kesterson <andrew@aklabs.net>
This commit is contained in:
2026-08-01 09:18:52 -04:00
parent 80ba6ddcfa
commit 1fd5062ddd
16 changed files with 230 additions and 62 deletions

View File

@@ -243,6 +243,48 @@ akerr_ErrorContext *akgl_actor_update(akgl_Actor *obj)
);
```
### Compiler warnings
**`-Wall` is on for every target this project owns** -- the library, the test
suites and `charviewer` -- and the tree builds clean under it at every
optimization level.
**`-Werror` is not on by default.** It is `option(AKGL_WERROR ... OFF)`, turned
on only by the `cmake_build` and `performance` CI jobs. Two reasons, and both
are measured rather than assumed:
- libakgl is consumed with `add_subdirectory()`; `akbasic` does exactly that. A
target-level `-Werror` makes every diagnostic a newer compiler invents into a
broken build for somebody else's project.
- The warning set is not stable across build types. An `-O2` build reports ten
`-Wstringop-truncation` warnings that `-O0` and `-fsyntax-only` do not,
because they need the middle end. The two CI jobs that set `AKGL_WERROR` are
deliberately one of each: coverage at `-O0`, performance at `-O2`.
If you are checking this by hand, **compile for real at the optimization level
being shipped.** `-fsyntax-only` reported 6 of the 16 findings in `src/`.
**`-Wextra` is deliberately not adopted.** It adds 22 findings, 17 of which are
inherent to the design rather than defects: 13 `-Wunused-parameter`, because a
backend vtable entry or an SDL callback must match a signature whether it reads
every argument or not, and 4 `-Wimplicit-fallthrough` from libakerror's own
`PROCESS`/`HANDLE`/`HANDLE_GROUP`, which fall through between `case` labels by
design. Adopting it today would mean disabling both permanently to gain 5
`-Wsign-compare`. The fallthrough half is filed upstream as
`deps/libakerror/TODO.md` item 8; revisit when that lands.
**Vendored code is exempt, not fixed.** `deps/semver/semver.c` is listed
directly in `add_library(akgl ...)`, so the target's `PRIVATE` options reach it;
`set_source_files_properties(... COMPILE_OPTIONS "-w")` keeps a future `semver`
update from failing this build. The other vendored projects are separate targets
and are unaffected either way.
**Never silence a warning with a cast.** Three `-Wpointer-sign` findings in
`src/sprite.c` were real signedness mismatches -- `uint32_t *` passed where an
`int *` was expected -- and a cast would have hidden every one. That is the
whole argument of `TODO.md` item 37. Read into a correctly typed local, check
the range, and assign.
### No repository-wide formatter
There is no `clang-format` or linter wired into the build, and `cc-mode`'s