andrew/libakgl
1
0
Fork 0
Code Issues Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

Fix the type, macro and state-table defects, and the leftover debris

Browse Source 
Closes internal-consistency items 19 through 36, 38 and 41. Item 37, the ~180
redundant casts, is deliberately left open with its reasoning in TODO.md: the
benefit only arrives once the build turns on the warnings those casts suppress,
and doing it before that is churn across the two files with the most
outstanding functional defects.

The two that were real bugs are in the actor state table.
AKGL_ACTOR_STATE_STRING_NAMES was declared [AKGL_ACTOR_MAX_STATES+1] and
defined [32], so a consumer trusting the declared bound read past the object;
and indices 11 and 12 were named UNDEFINED_11 and UNDEFINED_12 where actor.h
has MOVING_IN and MOVING_OUT, so no character JSON could bind a sprite to
either state. tests/registry.c now walks the whole table -- every entry
non-NULL, every entry resolving to its own bit, no two entries sharing a name.

The bitmask macros are parenthesized and AKGL_BITMASK_CLEAR has lost the
semicolon inside its body. Writing tests/bitmasks.c for that turned up
something worth knowing: the obvious test does not catch it. For a bit that is
set, the misparse `!(mask & bit) == bit` gives the same answer as the correct
one. It only diverges for an unset bit whose value is not 1, and that is the
shape the suite uses now.

akgl_draw_background was the last public function outside the error protocol.
It takes a backend like everything else in draw.h, restores the draw colour it
found, and is tested -- TODO.md had it filed under "needs the offscreen
renderer harness", which was never true; what it needed was to stop reading the
global.

All eight registry initializers go through one helper, so the seven that leaked
an SDL_PropertiesID on every call after the first no longer do. Fixed in the
same place because it is the same function: akgl_registry_init never called
akgl_registry_init_properties, which made akgl_set_property a silent no-op for
anyone not going through akgl_game_init -- Defects, Known and still open item 3.

Also: AKGL_COLLIDE_RECTANGLES (three open parens, two closes) and akgl_Frame
deleted, float32_t/float64_t used consistently, the developer-specific debug
logging removed from the controller inner loop, the abandoned SDL_GetBasePath
comments removed, nine unused locals removed, and dst renamed to dest.

akgl_game_update's default flags no longer OR the same bit twice. That changes
nothing today, and the reason is Performance item 32: the loop never reads
either bit, which is why every actor is updated sixteen times a frame. Still
open.

25/25 pass, memcheck clean, reindent --check clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Andrew Kesterson
2026-08-01 00:15:36 -04:00
parent a60220d39b
commit 19530f6a97
28 changed files with 735 additions and 398 deletions
Download Patch File Download Diff File Expand all files Collapse all files

275
TODO.md
View File

@@ -211,175 +211,172 @@ resolved for every pair it listed.
### 4. Types and macros
19. **`float`/`double` are used raw where `types.h` defines aliases.** `types.h`
exports `float32_t` and `float64_t`, and the actor and character structs use
`float32_t` throughout — but `akgl_get_json_number_value` takes `float *`
(`json_helpers.h:55`), `akgl_get_json_double_value` takes `double *` (line 66),
`akgl_PhysicsBackend`'s six drag/gravity fields are `double`
(`physics.h:22-27`), and `akgl_Tilemap`'s perspective fields are `float`
(`tilemap.h:105-108`). Either use the aliases consistently or delete them.
**Items 19 through 23 are resolved in 0.5.0.**
20. **`AKGL_COLLIDE_RECTANGLES` (`include/akgl/util.h:27`) has unbalanced
parentheses** — three opens, two closes — so any use is a syntax error. It has
no callers and duplicates `akgl_collide_rectangles`. Delete it.
19. **`float`/`double` used raw where `types.h` defines aliases.** The four
signatures and twelve struct fields that spelled them out now use
`float32_t` and `float64_t` like the actor and character structs. They are
plain typedefs, so this is a spelling change and not an ABI one.
`float64_t`'s doc no longer says "unused so far".
21. **Bitmask macros are unparenthesized.** `AKGL_BITMASK_HAS(x, y)` expands to
`(x & y) == y` with no outer parens, so `!AKGL_BITMASK_HAS(a, b)` parses as
`!(a & b) == b`. No in-tree caller negates it today (`AKGL_BITMASK_HASNOT`
exists for that), so this is latent rather than live. `AKGL_BITMASK_CLEAR(x)`
(`game.h:86`) also carries a trailing semicolon inside the macro body, so
normal use produces an empty statement. Parenthesize all five and drop the
semicolon.
20. **`AKGL_COLLIDE_RECTANGLES` had unbalanced parentheses.** Deleted. Three
opens against two closes meant any expansion was a syntax error, it had no
callers, and it duplicated `akgl_collide_rectangles`.
22. **The state and iterator bit macros mix forms.** `AKGL_ITERATOR_OP_UPDATE`
(`iterator.h:15`) is written `1` while its 31 siblings are `1 << n`, and none
of the `1 << n` values in `iterator.h` or `actor.h:15-49` are parenthesized.
The hand-maintained trailing bit-pattern comments in `actor.h:34-49` are also
wrong for the high word — they restart at `0000 0000 0000 0001` for bit 16
rather than showing the full 32-bit value.
21. **Bitmask macros were unparenthesized.** All five are fully parenthesized,
and `AKGL_BITMASK_CLEAR` no longer carries a semicolon inside its body.
23. **`akgl_Frame` (`game.h:27-31`) is defined and never used** anywhere in
`src/`, `include/`, `tests/`, or `util/`.
`tests/bitmasks.c` covers the composition cases, and writing them turned up
something worth recording: the obvious test does **not** catch this. For a
bit that is *set*, `!AKGL_BITMASK_HAS(mask, bit)` misparses to
`!(mask & bit) == bit`, which is `0 == bit` -- false, the same answer the
correct parse gives. It only diverges for a bit that is *not* set and whose
value is not 1: `!(0)` is 1, and `1 == 64` is false where the answer should
be true. The suite now uses that shape, and fails against the old macros.
22. **The state and iterator bit macros mixed forms.**
`AKGL_ITERATOR_OP_UPDATE` is `(1 << 0)` like its 31 siblings, and every
`1 << n` in `iterator.h` and `actor.h` is parenthesized, with the
hand-aligned value columns preserved.
The bit-pattern comments in `actor.h` are not wrong so much as unlabelled:
each shows the pattern *within its own 16-bit half*, which is why bit 16
looks like it restarts at bit 0. The section headings say so now. Rendering
the full 32-bit value instead would push those lines past the 100-column
fill.
**Newly recorded, and still open:** `1 << 31` is undefined behaviour on a
signed `int`. It is `(1 << 31)` in both tables and wants to be an unsigned
shift, but `akgl_Actor::state` is `int32_t` and `akgl_Iterator::flags` is
`uint32_t`, so the two tables do not want the same answer. Worth deciding
deliberately rather than sneaking a `u` in.
23. **`akgl_Frame` was defined and never used.** Deleted.
### 5. `AKGL_ACTOR_STATE_STRING_NAMES` disagrees with `actor.h`
24. **The array bound differs between declaration and definition.**
`include/akgl/actor.h:57` declares `[AKGL_ACTOR_MAX_STATES+1]` (33);
`src/actor_state_string_names.c:6` defines `[32]`. Any consumer that trusts
the declared bound and reads index 32 reads past the object.
**All three resolved in 0.5.0.**
25. **Two entries name the wrong bit.** `actor.h` assigns bit 11 to
`AKGL_ACTOR_STATE_MOVING_IN` and bit 12 to `AKGL_ACTOR_STATE_MOVING_OUT`, but
`actor_state_string_names.c:18-19` puts `"AKGL_ACTOR_STATE_UNDEFINED_11"` and
`"AKGL_ACTOR_STATE_UNDEFINED_12"` at those indices — names for macros that do
not exist. Since `akgl_registry_init_actor_state_strings`
(`src/registry.c:79-94`) builds `AKGL_REGISTRY_ACTOR_STATE_STRINGS` from this
array and `akgl_character_load_json_state_int_from_strings`
(`src/character.c:101`) resolves character-JSON state names through it, a
character JSON can never bind a sprite to `MOVING_IN` or `MOVING_OUT`.
24. **The array bound differed between declaration and definition.** The
header declared `[AKGL_ACTOR_MAX_STATES+1]` (33) and the definition was a
literal `[32]`, so a consumer trusting the declared bound read past the
object. Both are `[AKGL_ACTOR_MAX_STATES]` now, and the definition is sized
by the macro rather than by a literal.
26. **The generation comment is stale.** `actor.h:53-55` says the file "is built
by a utility script and not kept in git, see the Makefile for
lib_src/actor_state_string_names.c". There is no Makefile (the project is
CMake), no `lib_src/`, no such script under `scripts/` or `util/`, and the
file *is* tracked in git at `src/actor_state_string_names.c`. Either restore
the generator — which would fix items 24 and 25 by construction — or delete
the comment and maintain the file by hand.
25. **Two entries named the wrong bit.** Indices 11 and 12 said
`AKGL_ACTOR_STATE_UNDEFINED_11` and `_12` where `actor.h` has `MOVING_IN`
and `MOVING_OUT`, so no character JSON could ever bind a sprite to either
state -- the name it would have to write was not in the registry.
`tests/registry.c` now walks the whole table: every entry non-`NULL`, every
entry resolving to its own bit through
`AKGL_REGISTRY_ACTOR_STATE_STRINGS`, no two entries sharing a name (a
duplicate silently overwrites and makes one bit unreachable), and
`MOVING_IN`/`MOVING_OUT` named explicitly so a regression reads as what it
is.
26. **The generation comment was stale.** There is no generator, no Makefile
and no `lib_src/`. The comment is gone and the file's own header now says it
is maintained by hand and states the two invariants that keep breaking.
### 6. Doxygen drift
27. **Three struct doc comments in `tilemap.h` are rotated by one.**
`akgl_TilemapObject` (line 31) is documented as "Stores tileset metadata,
texture, and frame offsets" (that is `akgl_Tileset`); `akgl_TilemapLayer`
(line 46) as "Represents an object embedded in a tilemap layer" (that is
`akgl_TilemapObject`); `akgl_Tileset` (line 61) as "Stores tile, image, or
object data for one map layer" (that is `akgl_TilemapLayer`).
27. **Three struct doc comments in `tilemap.h` were rotated by one.** Already
correct -- the doxygen rewrite fixed this before it was checked here.
`akgl_TilemapObject`, `akgl_TilemapLayer` and `akgl_Tileset` each describe
themselves.
28. **`point` is documented as "Represents a two-dimensional point"**
(`util.h:12`) but has `x`, `y`, and `z`.
28. **`point` documented as two-dimensional with an `x`, `y` and `z`.** Already
correct, and the type is `akgl_Point` now.
29. **Doc comments live on the definition for public functions.** The convention
is header-side documentation, but `akgl_path_relative_root` (`util.c:23`),
`akgl_path_relative_from` (`util.c:97`), the four `gamepad_handle_*`
(`controller.c:114+`), the `akgl_game_save_*` iterators and
`akgl_game_load_*` helpers (`game.c:173+`), and the tilemap helpers
(`tilemap.c:90+`) are documented only in the `.c`. This is the same set as
item 7 — resolving that resolves this.
29. **Doc comments on the definition rather than the header.** Resolved with
item 7: the four controller handlers and the six tilemap loader helpers had
their documentation moved to the header when they were declared there.
### 7. Formatting
### 7. Formatting and hygiene
30. **A minority of files use a 2-column offset instead of the canonical
4-column one.** The mix of tabs and spaces across most of `src/` is *not*
disorder: it is exactly what Emacs `cc-mode` emits for the `stroustrup` style
with `indent-tabs-mode t` and `tab-width 8` — depth 1 is four spaces, depth 2
is one tab, depth 3 is a tab plus four spaces, and so on. Twelve of the
seventeen `.c` files follow that ladder cleanly.
**Items 31 through 36, 38 and 41 are resolved in 0.5.0. Item 37 is not; see
below.**
The genuine outliers indent at 2 columns: `src/json_helpers.c` (82 lines,
except `akgl_get_json_with_default` at line 149 which is 4), `src/util.c`
(37 lines, from `akgl_rectangle_points` at line 118 onward), `src/assets.c`
(9), `src/staticstring.c` (7), `src/actor.c` (8, in `akgl_actor_add_child`
at line 272), plus `include/akgl/util.h` (7) and
`include/akgl/staticstring.h` (2).
31. **Leftover debug code.** The four `SDL_Log` lines in `src/controller.c`
guarded by `event->type == 768 && event->key.which == 11 &&
event->key.key == 13` -- decimal literals for one keyboard on one
developer's machine, inside the per-event inner loop -- are gone.
**Resolved.** `AGENTS.md` now specifies the canonical style and the exact
`cc-mode` settings; `.dir-locals.el` applies them in Emacs; and
`scripts/reindent.sh` applies them in batch. The whole tree (32 files across
`src/`, `include/`, `tests/`, and `util/`) has been reindented and is a fixed
point of `scripts/reindent.sh --check`. Verified whitespace-only: apart from
three trailing blank lines removed at EOF in `src/heap.c`, `src/registry.c`,
and `tests/tilemap.c`, `git diff -w` over the reindent is empty, and the test
results are unchanged (13/14, `character` still the intentional failure).
`scripts/hooks/pre-commit` keeps it that way — enable with
`git config core.hooksPath scripts/hooks`.
32. **Large commented-out blocks.** The five abandoned `SDL_GetBasePath()`
path-prefixing lines across `assets.c`, `sprite.c`, `character.c` and
`tilemap.c` are gone. They were superseded by `akgl_path_relative`.
31. **Leftover debug code ships in the library.** `src/controller.c:91-97` logs
four lines whenever `event->type == 768 && event->key.which == 11 &&
event->key.key == 13` — hardcoded decimal values for a specific keyboard ID
on a specific developer's machine, inside the per-event inner loop.
33. **Unused locals.** `curTime` in `akgl_game_update` and in
`akgl_render_2d_draw_world`, `j` in `akgl_render_2d_draw_world` (shadowed
by its own inner loop), `target` in `akgl_character_sprite_get`, both
`result` declarations in `util.c`, and `screenwidth`/`screenheight` in
`akgl_game_init`. `opflags` in `akgl_heap_release_character` is no longer
unused -- it is what drives the state-sprite walk added for **Defects**
item 21.
32. **Large commented-out blocks.** `src/sprite.c:115-120,157,185-198,210`;
`src/character.c:192-199,215`; `src/assets.c:18-27,50`;
`src/tilemap.c:583,596-598,640`. All are the same abandoned
`SDL_GetBasePath()` path-prefixing approach, superseded by
`akgl_path_relative`. Delete them.
34. **`akgl_game_update`'s default flags OR-ed the same bit twice.** It reads
`AKGL_ITERATOR_OP_UPDATE | AKGL_ITERATOR_OP_LAYERMASK` now. This is a
statement of intent rather than a behaviour change, and the reason is worth
knowing: **nothing in that loop reads either bit**. It never compares
`actor->layer` to the layer it is sweeping, which is exactly **Performance**
item 32 -- every actor updated sixteen times a frame -- and that is still
open.
33. **Unused locals.** `screenwidth`/`screenheight` (`game.c:53-54`), `curTime`
(`game.c:499`), `curTime` and `j` (`renderer.c:114,116` — `j` is also shadowed
by the inner loop at line 128), `target` (`character.c:59`), `result`
(`util.c:37,73`), `opflags` (`heap.c:146`, declared and cleared but never
read).
35. **`akgl_draw_background` was the only public function outside the error
protocol.** It now takes an `akgl_RenderBackend *` like every other entry
point in `draw.h`, returns an error context, checks the backend and its
`sdl_renderer`, and restores the draw colour it found instead of leaving it
changed.
34. **`akgl_game_update`'s default flags OR the same bit twice.**
`src/game.c:496` reads
`(AKGL_ITERATOR_OP_LAYERMASK | AKGL_ITERATOR_OP_LAYERMASK)`. Given the loop
that follows walks layers and calls `updatefunc`, the intent was almost
certainly `AKGL_ITERATOR_OP_UPDATE | AKGL_ITERATOR_OP_LAYERMASK`.
It was listed under **Remaining work** as needing the offscreen renderer
harness. It does not, and never did -- what it needed was to stop reading
the global. `tests/draw.c` covers the checkerboard pattern, the restored
draw colour, zero and negative sizes, and a `NULL` backend.
35. **`akgl_draw_background` is the only public function outside the error
protocol.** `include/akgl/draw.h:14` returns `void` and reports nothing;
every other public entry point returns `akerr_ErrorContext *`. It also calls
`SDL_SetRenderDrawColor` and `SDL_RenderFillRect` without checking `renderer`
or `renderer->sdl_renderer`.
36. **`akgl_registry_init_actor` was the only initializer that destroyed the
registry it was replacing.** All eight go through one `registry_create()`
helper now, so the other seven stop leaking an `SDL_PropertiesID` per call
after the first -- which a game that resets between levels makes on every
level.
36. **`akgl_registry_init_actor` is the only registry initializer that destroys
an existing registry** before recreating it (`src/registry.c:47-49`). The
other seven leak the old `SDL_PropertiesID` on a second call. Either all of
them should do it or none should.
Fixed alongside it, because it is the same function: `akgl_registry_init()`
never called `akgl_registry_init_properties()`, which is **Defects → Known
and still open** item 3. `AKGL_REGISTRY_PROPERTIES` stayed 0 for any caller
that did not also go through `akgl_game_init`, making `akgl_set_property` a
silent no-op and `akgl_get_property` always return the caller's default --
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.** `(json_t *)json` where `json` is
already `json_t *`, `(akgl_Tilemap *)dest`, `(akgl_Actor *)actorobj`,
`(char *)&obj->name` on an array that already decays — roughly 180 pointer
casts across `src/`, a large majority of them no-ops, densest in
`src/tilemap.c:150-624` and `src/character.c:144-172`.
They suppress exactly the conversion warnings that would catch a real
mismatch.
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`.
38. **`struct`-qualified parameters in two definitions.**
`akgl_physics_null_move` (`src/physics.c:29`) and `akgl_physics_arcade_move`
(`src/physics.c:80`) are defined with `struct akgl_PhysicsBackend *self`
while the header and the other eight physics functions use the typedef.
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.
39. **`text.c:19` validates the wrong argument.**
`FAIL_ZERO_RETURN(errctx, name, AKERR_NULLPOINTER, "Null filepath")` checks
`name` a second time; `filepath` is never checked and is passed straight to
`TTF_OpenFont`. Copy-paste of line 18.
**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.
**Resolved** alongside the text measurement work; `tests/text.c` asserts a
NULL filepath reports `AKERR_NULLPOINTER`.
38. **`struct`-qualified parameters in two definitions.** `akgl_physics_null_move`
and `akgl_physics_arcade_move` use the typedef like the other eight.
40. **`akgl_path_relative` and `akgl_path_relative_from` disagree on the output
parameter.** **Moot in 0.5.0**: `akgl_path_relative_from` is deleted (see
**Defects → Known and still open** item 4) and `akgl_path_relative_root` is
`static`, so `akgl_path_relative` is the only member of the family left and
there is nothing left to disagree with. It still takes `akgl_String *dst`
rather than the `**` form the rest of the library uses, which stays as
**item 41**'s question of naming and this one's question of shape -- but
with one function it is a decision to make when something needs it, not a
drift between siblings.
39. **`text.c` validated the wrong argument.** Resolved earlier, alongside the
text measurement work.
40. **`akgl_path_relative_from` disagreed on the output parameter.** Moot: the
function is deleted. See **Defects → Known and still open** item 4.
41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and
`akgl_path_relative` take `dest` now, like everything else.
41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and the
`akgl_path_relative*` family use `dst`; everything else uses `dest`.
## Test suites that could not fail