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>
This commit is contained in:
54
TODO.md
54
TODO.md
@@ -358,22 +358,25 @@ below.**
|
||||
so `akgl_physics_init_arcade` and `akgl_render_2d_init` quietly ignored
|
||||
their configuration. `tests/registry.c` sets a property and reads it back.
|
||||
|
||||
37. **Redundant casts obscure the code.** **Still open, and deliberately so.**
|
||||
Roughly 180 pointer casts across `src/`, a large majority no-ops --
|
||||
`(json_t *)json` where `json` is already `json_t *`, `(char *)&obj->name` on
|
||||
an array that decays anyway -- densest in `src/tilemap.c` and
|
||||
`src/character.c`.
|
||||
37. **Redundant casts obscure the code.** **Still open -- but the precondition
|
||||
it named is now met.** Roughly 180 pointer casts across `src/`, a large
|
||||
majority no-ops, densest in `src/tilemap.c` and `src/character.c`.
|
||||
|
||||
The argument for fixing it is real: a no-op cast suppresses exactly the
|
||||
conversion warning that would catch a mismatch. The argument for not doing
|
||||
it *here* is that it is the largest and least mechanical change on this
|
||||
list, it touches almost every line of the two files with the most
|
||||
outstanding functional defects, and the benefit only arrives once the build
|
||||
actually turns those warnings on -- which it does not today. Doing this
|
||||
without `-Wall -Wextra` on the library target buys nothing but churn.
|
||||
This item used to say the benefit only arrives once the build turns on the
|
||||
warnings those casts suppress, and that doing it first buys nothing but
|
||||
churn. `-Wall` is on now (see `AGENTS.md`, "Compiler warnings"), so the
|
||||
sweep can proceed as its own commit.
|
||||
|
||||
**Do them together**, in that order: enable the warnings, see what they say,
|
||||
then remove the casts that were hiding something and the ones that were not.
|
||||
**The argument turned out to be right, with evidence.** Turning `-Wall` on
|
||||
found three genuine signedness mismatches in `src/sprite.c`: `obj->width`,
|
||||
`obj->height` and `obj->speed` are `uint32_t` and were being passed straight
|
||||
to `akgl_get_json_integer_value(..., int *)`. A cast would have silenced all
|
||||
three. They are fixed by reading into an `int`, range-checking, and
|
||||
assigning -- which also turned up that `speed` is scaled by 1,000,000 into a
|
||||
32-bit field, so anything past 4294 ms overflowed rather than being held.
|
||||
|
||||
When doing the sweep: remove a cast, rebuild, and read what the compiler
|
||||
says. A cast that was load-bearing will say so.
|
||||
|
||||
38. **`struct`-qualified parameters in two definitions.** `akgl_physics_null_move`
|
||||
and `akgl_physics_arcade_move` use the typedef like the other eight.
|
||||
@@ -1858,6 +1861,29 @@ this library and told it is compatible.
|
||||
outlived its own press, the code-point-boundary truncation, and both NULL arguments.
|
||||
`src/controller.c` holds at 91%.
|
||||
|
||||
## Truncated registry keys can collide
|
||||
|
||||
Found while turning `-Wall` on. `akgl_actor_initialize` and
|
||||
`akgl_character_initialize` document their name fields as "Truncated, not
|
||||
rejected, if the source name is longer", and that is what they do -- the copy is
|
||||
`aksl_strncpy` bounded to the field, so it always terminates now.
|
||||
|
||||
Termination was the overread half, and it is fixed. The other half is not: the
|
||||
truncated name **is the registry key**. Two distinct 200-character names
|
||||
truncate to the same 127-byte key, and the second `SDL_SetPointerProperty`
|
||||
silently replaces the first. The objects are different; the registry cannot tell.
|
||||
|
||||
The same applies to `akgl_Sprite::name` (128), `akgl_SpriteSheet::name` (512)
|
||||
and the tilemap's object and tileset names.
|
||||
|
||||
Whether that matters depends on whether long asset names are realistic, and 127
|
||||
bytes is generous for a hand-written name in a JSON file. Recording it rather
|
||||
than fixing it, because the fix is a contract change -- refuse an over-long name
|
||||
with `AKERR_OUTOFBOUNDS` instead of truncating -- and every one of those headers
|
||||
currently promises the opposite. `aksl_strncpy` already reports exactly that
|
||||
status when the bytes do not fit, so the change is to stop capping `n` at
|
||||
`size - 1` and let it raise.
|
||||
|
||||
## Carried over
|
||||
|
||||
1. **Make character-to-sprite state bindings release their references symmetrically.**
|
||||
|
||||
Reference in New Issue
Block a user