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

Record the fixes whose TODO.md edits were dropped

Browse Source 
Eleven entries still described the old code: known-and-still-open items 1, 2,
5, 9 and 11, error-handling items 16, 17 and 18, and defects 20, 29 and 30. The
code and the tests were right; the document was not.

The edits were lost the same way each time. The scripts that made them built
the whole file in memory and wrote it once at the end, so an assertion failure
partway through -- on an entry whose text had already been reworded -- threw
away that script's earlier, correct replacements along with the failed one.
Written one entry at a time now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Andrew Kesterson
2026-08-01 06:28:34 -04:00
parent 3f37d38f2d
commit 3ee8c60491
1 changed files with 108 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

182
TODO.md
View File

@@ -176,38 +176,48 @@ resolved for every pair it listed.
### 3. Error-handling pattern
16. **`*_RETURN` macros are used inside `ATTEMPT` blocks, which skips `CLEANUP`.**
The established pattern is `FAIL_*_BREAK` / `CATCH` inside `ATTEMPT` and
`*_RETURN` outside it. Violations:
- `src/tilemap.c:52` — `SUCCEED_RETURN` on the success path of
`akgl_get_json_tilemap_property` returns directly from inside `ATTEMPT`,
bypassing the `CLEANUP` at line 54 that releases `tmpstr` and `typestr`.
Every successful property lookup leaks two heap strings, and the string
pool is only 256 entries.
- `src/tilemap.c:620` — `FAIL_RETURN` inside `ATTEMPT`.
- `src/controller.c:286-289,306` — `FAIL_ZERO_RETURN` / `FAIL_NONZERO_RETURN`
inside `ATTEMPT`; `src/controller.c:383` — `SUCCEED_RETURN` inside `ATTEMPT`,
which also leaves `akgl_controller_default` with no return statement on the
path that falls out of `FINISH`.
- `src/game.c:457,462` — `FAIL_NONZERO_RETURN` inside `ATTEMPT`, skipping the
`fclose` in `CLEANUP` at line 482.
**Fixed in 0.5.0.** Ten sites, found by scanning rather than from this list
-- it named six and missed the four in `akgl_collide_rectangles`.
17. **NULL-check discipline varies by function.** In `src/json_helpers.c` only
`akgl_get_json_string_value` (line 71) and `akgl_get_json_array_index_string`
(line 128) validate `key`/`dest`; the other eight accessors validate only the
container and then dereference `dest` unconditionally. In `src/physics.c` the
arcade backends check `actor` but `akgl_physics_null_gravity`,
`_null_collide`, and `_null_move` (lines 15-34) check only `self`. In
`src/renderer.c:65,74`, `akgl_render_2d_frame_start` and `_frame_end`
dereference `self->sdl_renderer` with no check on `self`, while
`akgl_render_2d_draw_texture` (line 82) checks `self` first.
The one that mattered was the success path of
`akgl_get_json_tilemap_property`, which leaked two of the string pool's 256
entries on every lookup that *found* what it was asked for. A map load does
that several times per layer.
18. **Error-context variable naming is split between `errctx` and `e`,
sometimes within one file.** `src/util.c` uses `e` in the path helpers
(lines 32-116) and `errctx` in the geometry helpers (lines 118-259);
`src/heap.c` uses `errctx` everywhere except `akgl_heap_init_actor` (line 50).
`src/actor.c`, `character.c`, `json_helpers.c`, `registry.c`, `sprite.c`, and
`tilemap.c` favor `errctx`; `game.c`, `physics.c`, `renderer.c`, and
`controller.c` favor `e`. Pick one.
Two needed more than swapping the macro. In
`akgl_get_json_tilemap_property` a plain `break` would have fallen through
to the "property not found" `FAIL_RETURN` after `FINISH`, reporting a miss
for something found, so the success path sets a flag. In
`akgl_collide_rectangles` the eight early exits were followed by
`*collide = false;`, which would have overwritten the hit that broke out of
the block; each corner test writes the flag itself, so that line is gone
rather than moved.
`akgl_controller_default` was the other behavioural one: its
`SUCCEED_RETURN` was the last statement in the `ATTEMPT` block, so the path
that falls out of `FINISH` reached the closing brace of a non-void function.
**`scripts/check_error_protocol.py` keeps this closed**, as the
`error_protocol` test. It also enforces the other rule with a silent failure
mode -- no `return` out of a `HANDLE` block.
17. **NULL-check discipline varies by function.** **Fixed in 0.5.0.** The eight
typed JSON accessors that validated their container and then wrote through
`dest` unconditionally now check `key` and `dest` as the two string
accessors always did; the null physics backend checks its actors like the
arcade one; and `akgl_render_2d_frame_start`, `_frame_end` and `_shutdown`
check `self`, which the first two read straight through.
`tests/renderer.c` calls all three with `NULL`, which segfaulted before.
18. **Error-context variable naming is split between `errctx` and `e`.**
**Fixed in 0.5.0**, in its own commit because it is a rename and nothing
else. All 45 remaining sites are `errctx`; applying `\be\b -> errctx` to
each removed line reproduces the added line exactly, and the counts match at
333 either way.
`e` keeps its meaning where the convention wants it -- an incoming context
being inspected, as in `akgl_get_json_with_default(e, ...)`.
### 4. Types and macros
@@ -658,13 +668,20 @@ Each was found by a test written to assert correct behavior.
### Known and still open
1. **`akgl_render_and_compare` compares a texture against itself.**
`src/util.c:228` and `src/util.c:245` both draw `t1`; `t2` is never rendered,
so the function always passes and the image assertions in `tests/sprite.c`
assert nothing.
**Fixed in 0.5.0**: the second pass draws `t2`. Both passes drew `t1`, so it
always reported a match and every image assertion built on it -- including
the ones in `tests/sprite.c` -- asserted nothing.
2. **`akgl_tilemap_release` double-frees tileset textures.**
`src/tilemap.c:849` destroys `dest->tilesets[i].texture` inside the *layers*
loop; it should be `dest->layers[i].texture`. It also never NULLs the
pointers, so a second release is a use-after-free.
**Fixed in 0.5.0.** The layers loop tested `layers[i].texture` and destroyed
`tilesets[i].texture`, so every tileset texture was freed twice on a single
release and no image layer's texture was freed at all. Each pointer is
cleared as it goes now, which also makes a second release safe rather than a
use-after-free.
`tests/tilemap.c` loads the fixture map, releases it three times, and
asserts every texture pointer is `NULL`.
3. **`akgl_registry_init` never initializes the properties registry.**
**Fixed in 0.5.0**, alongside internal-consistency item 36, which is the same
function. `akgl_registry_init()` calls `akgl_registry_init_properties()` now,
@@ -681,8 +698,11 @@ Each was found by a test written to assert correct behavior.
closes item 40, which was about the output-parameter shape it disagreed with
the rest of the family on.
5. **`akgl_compare_sdl_surfaces` memcmps without checking geometry.**
`src/util.c:208` compares `s1->pitch * s1->h` bytes of `s2` without verifying
the surfaces share dimensions, pitch, or format.
**Fixed in 0.5.0**: dimensions, pitch and pixel format are compared first,
and any difference is a mismatch. `tests/util.c` compares a 32x32 surface
against an 8x8 one in both directions, which used to read 4 KiB past the end
of the smaller.
6. **`akgl_string_initialize` overflows by four bytes when `init` is NULL.**
**Fixed in 0.5.0**: it zeroes `sizeof(obj->data)`. The four bytes it used to
run past the end of the object landed on the *next* pool slot's `refcount`,
@@ -709,14 +729,16 @@ Each was found by a test written to assert correct behavior.
`next_character` do not. `tests/heap.c` pins the current behavior and says so;
decide whether to make them symmetric or document the split.
9. **`tests/util.c` defines `test_akgl_collide_point_rectangle_logic` but
`main()` never calls it.**
`main()` never calls it.** **Fixed in 0.5.0**; it is called, and passes.
10. **`controller.h` declares functions that do not exist.** **Fixed in 0.5.0**;
the definitions carry the declared `akgl_controller_handle_*` names now. See
internal-consistency item 7, and `scripts/check_api_surface.sh`, which is
what stops this class of drift coming back.
11. **`akgl_controller_pushmap` and `akgl_controller_default` accept negative map
ids.** Both check `controlmapid >= AKGL_MAX_CONTROL_MAPS` but not
`controlmapid < 0`, so a negative id indexes before `GAME_ControlMaps`.
ids.** **Fixed in 0.5.0**: both check the lower bound as well.
`tests/controller.c` passes -1 and -4096 to each.
12. **A failed controller-DB fetch silently destroys the tracked fallback.**
`include/akgl/SDL_GameControllerDB.h` is committed deliberately so the
library still builds if upstream disappears. But `mkcontrollermappings.sh`
@@ -879,14 +901,18 @@ without coming here first. Ordered by blast radius.
`CLEANUP`. Touches `src/assets.c:20-47`.
20. **`akgl_character_sprite_add` leaks a sprite reference when a state is
remapped.** `src/character.c:51` writes the new sprite over any existing
entry for that state without releasing the one it displaces, whose refcount
was incremented when it was added. The displaced sprite's pool slot is never
reclaimed. The write itself is also unchecked, so a failure to record the
mapping is reported as success.
remapped.** **Fixed in 0.5.0**: it reads the existing entry first and
releases it once the new binding is recorded, and the write is checked.
Fix: read the existing entry first and `akgl_heap_release_sprite` it, and
check `SDL_SetPointerProperty`'s return. Touches `src/character.c:43-55`.
The new reference is taken *before* the write and given back if the write
fails, so there is no window where a sprite is bound with nothing behind it.
Rebinding a state to the sprite already there is not treated as a
displacement.
`tests/character.c` binds, rebinds, and then runs 200 alternating rebinds,
asserting the displaced sprite's count comes back each time. This is the
half **Carried over** item 1 calls replacement; there is still no API to
*remove* a binding without replacing it.
21. **`akgl_heap_release_character` abandons the whole state-to-sprite map.**
`src/heap.c:150` zeroed the character without walking `state_sprites`, so
@@ -1012,37 +1038,45 @@ Ordered by blast radius. Numbering continues the **Defects** list above.
error-handling protocol, which warns about `*_RETURN` inside `ATTEMPT` but
not about returning out of `HANDLE`.
29. **`akgl_tilemap_load` leaks five pooled strings per load.** Measured by
counting non-zero `HEAP_STRING` refcounts across load/release cycles: five
per cycle, exactly, and `akgl_tilemap_release` gives none of them back. The
52nd map load in a process finds the pool empty.
29. **`akgl_tilemap_load` leaks five pooled strings per load.** **Fixed in
0.5.0**, in two steps, and the split changes what the number means.
Blast radius: every level transition. A game with fifty levels, or one that
reloads a level on death, hits it. The fix is a sweep of the loader's
`CLEANUP` blocks — `akgl_tilemap_load`, `akgl_tilemap_load_layers`,
`akgl_tilemap_load_layer_tile`, `akgl_tilemap_load_layer_objects`, and
`akgl_tilemap_load_tilesets_each` all claim scratch strings — and it wants a
test that asserts the pool is unchanged across a load/release cycle. That is
its own commit, not a footnote to a benchmark.
Three of the five were `akgl_get_json_tilemap_property` leaking two scratch
strings on every *successful* property lookup, through a `SUCCEED_RETURN`
inside its `ATTEMPT` block -- internal-consistency item 16. That took the
measured leak from five per load to two.
Until it is fixed, the tilemap-load benchmark in `tests/perf_render.c`
reclaims the pool by hand between iterations, and says so.
The remaining two were each a claim with no matching release:
`akgl_tilemap_load_layers` never gave back the string it read every layer's
`type` into, and `akgl_tilemap_load` never gave back the `dirname` its
relative paths resolve against. Both release in `CLEANUP` now.
`tests/tilemap.c` asserts the pool is exactly where it started after one
load/release cycle and after 64 -- enough that a leak of one string per load
could not finish. Finding the last two meant dumping the contents of every
still-claimed slot after a cycle rather than reading the code again;
`'tilelayer'` and an assets directory named themselves immediately.
Fixed alongside, same file and same class:
`akgl_tilemap_load_layer_objects` released its scratch string after reading
each object's name and then kept using the slot -- and
`akgl_get_json_string_value` reuses a non-`NULL` destination *without*
taking another reference, so the slot was free while still live and any
other claim could have been handed it.
The tilemap-load benchmark in `tests/perf_render.c` no longer needs to
reclaim the pool by hand between iterations.
30. **Two JSON accessors turn string-pool exhaustion into a segfault.**
`akgl_get_json_string_value` ends its `ATTEMPT` with
`FINISH(errctx, false)` at `src/json_helpers.c:89`, so a failed
`akgl_heap_next_string` is swallowed rather than passed up, and
`src/json_helpers.c:91` then `strncpy`s `AKGL_MAX_STRING_LENGTH` bytes
through the pointer it never set. `akgl_get_json_array_index_string` has the
same pair at `src/json_helpers.c:143` and `src/json_helpers.c:145`. Item 29
is what makes this reachable, and this is what item 29 looks like from the
outside: not `AKGL_ERR_HEAP`, a crash inside `strncpy` with a NULL
destination.
**Fixed in 0.5.0**: `FINISH(errctx, true)` in both, so a failed
`akgl_heap_next_string` reaches the caller instead of being swallowed and
then `strncpy`d through.
Fix: `FINISH(errctx, true)` in both. One word each, but it changes what
callers see on a path they currently cannot survive, so it wants a test that
exhausts the pool deliberately and asserts `AKGL_ERR_HEAP` comes back out of
both functions.
`tests/json_helpers.c` claims every slot in the pool and asserts
`AKGL_ERR_HEAP` comes back out of both accessors with the destination left
untouched. Against the old code that test segfaults rather than failing --
and so did the new tilemap property-lookup test, which is how this was
confirmed rather than merely believed.
31. **`akgl_game_update` segfaults if `akgl_game_init` did not run.**
**Fixed in 0.5.0**: `akgl_game_update_fps` installs `akgl_game_lowfps` when