Six findings, all of them libakgl's, all closed. The memcheck run is clean. Not one of the four json_load_file calls in src/ was ever matched by a json_decref, so every asset load abandoned its parsed document: 1.5 KB per sprite, 2.1 KB per character, 9 KB per load of the 2x2 fixture map, and on the order of a megabyte for a real one. Each loader now releases it in the CLEANUP block it already had, on the success path as well as the failure one. akgl_registry_load_properties needed its loop moved inside the ATTEMPT block first: props is a borrowed reference into the document and is read after the block ends, so every exit from that loop leaked the whole tree. akgl_get_property copied a fixed AKGL_MAX_STRING_LENGTH bytes out of what SDL handed back, which is SDL's strdup of the value -- four bytes for "0.0". That read up to 4 KiB past the end of somebody else's allocation on every property read, returned whatever was there past the terminator, and would have faulted on a value that landed at the end of a page. It copies the value and its terminator now, and refuses a value too long for an akgl_String rather than truncating it into an unterminated buffer. The header note that described the overread as a quirk describes correct behaviour instead. The four savegame name tables wrote a fixed-width field starting at the registry key, and SDL sizes that allocation to the name. They read past it on every entry and put what they found into the save file: up to half a kilobyte of this process's heap per registered object, in a file a player might send to somebody. They stage through a zeroed buffer now, and a negative-array-size typedef fails the build if a table's width ever outgrows it. akgl_controller_list_keyboards never freed the array SDL_GetKeyboards allocated for it. A font could be opened and published and never handed back -- there was no way to close one, so a game that changed fonts between scenes leaked ten kilobytes each time, and loading over a live name leaked the font it displaced. akgl_text_unloadfont is that missing half, and akgl_text_loadfont calls it when it replaces a name, after the new font has opened so a failed reload leaves the caller with the font they had. A new public symbol takes the version to 0.4.0 and the soname with it: an 0.3 consumer cannot be handed this library and told it is the same ABI. tests/registry.c fills a destination with a sentinel and asserts the bytes past the terminator survive a read, which fails against the old copy. tests/text.c covers unload, double unload, unloading a name that was never registered, and replacement closing the displaced font. The JSON releases have no test of their own and cannot sensibly have one -- nothing in the public API can observe a jansson refcount -- so the memcheck run is their test, which is an argument for gating it rather than against. The two remaining findings are in deps/semver's own unit test, which is vendored. They are suppressed by function name, so a rewrite of those cases comes back as a finding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1716 lines
103 KiB
Markdown
1716 lines
103 KiB
Markdown
# TODO
|
|
|
|
## Internal consistency
|
|
|
|
Findings from a sweep of `src/` and `include/`. These are consistency and
|
|
convention problems, not new functional defects — where one has a functional
|
|
consequence it is called out. Ordered roughly by blast radius. Items that
|
|
overlap the existing **Defects** list are cross-referenced rather than repeated.
|
|
|
|
### 1. Public naming conventions
|
|
|
|
1. **Include guards use three different schemes.** `_AKGL_ACTOR_H_`,
|
|
`_AKGL_CHARACTER_H_`, `_AKGL_GAME_H_`, `_AKGL_HEAP_H_`, `_AKGL_ITERATOR_H_`,
|
|
`_AKGL_SPRITE_H_`, and `_AKGL_TYPES_H_` carry the project prefix; `_ASSETS_H_`,
|
|
`_CONTROLLER_H_`, `_DRAW_H_`, `_ERROR_H_`, `_JSON_HELPERS_H_`, `_PHYSICS_H_`,
|
|
`_REGISTRY_H_`, `_RENDERER_H_`, `_TEXT_H_`, `_TILEMAP_H_`, and `_UTIL_H_` do
|
|
not. Pick `_AKGL_<FILE>_H_` everywhere.
|
|
|
|
The worst case is `include/akgl/staticstring.h:6`, which guards with
|
|
`_STRING_H_` — a name several libc implementations use for their own
|
|
`<string.h>`. The same file then does `#include "string.h"` (line 9) with
|
|
quotes, which is a relative-first lookup that only reaches the system header
|
|
by accident. Rename the guard to `_AKGL_STATICSTRING_H_` and use
|
|
`#include <string.h>`.
|
|
|
|
2. **Function names contradict the stated convention.** `AGENTS.md` says public
|
|
symbols use `akgl_` and types use `akgl_TypeName`. Exceptions:
|
|
- `akgl_Actor_cmhf_left_on` and its seven siblings (`include/akgl/actor.h:196-252`)
|
|
embed the *type* name in a *function* name, unlike every other actor entry
|
|
point (`akgl_actor_*`). Rename to `akgl_actor_cmhf_*`.
|
|
- `akgl_game_updateFPS` (`include/akgl/game.h:104`) is camelCase; every other
|
|
function is snake_case.
|
|
- `akgl_render_init2d` (`include/akgl/renderer.h:83`) puts the `2d` at the end
|
|
while the six functions it installs are `akgl_render_2d_*`. Make it
|
|
`akgl_render_2d_init`.
|
|
- `akgl_sprite_sheet_coords_for_frame` (`include/akgl/sprite.h:86`) spells it
|
|
`sprite_sheet`; `akgl_spritesheet_initialize` and `akgl_heap_next_spritesheet`
|
|
spell it `spritesheet`.
|
|
|
|
3. **Two public types have no prefix at all.** `point` and `RectanglePoints`
|
|
(`include/akgl/util.h:13-25`) are unprefixed, and `RectanglePoints` is
|
|
PascalCase with no namespace. Both are dumped into every translation unit
|
|
that includes `util.h`. Rename to `akgl_Point` / `akgl_RectanglePoints`.
|
|
|
|
4. **Global variables use four different conventions.** `window`, `bgm`, `game`,
|
|
`gamemap`, `renderer`, `physics`, and `camera` (`src/game.c:26-43`) are
|
|
unprefixed single common words exported from a shared library — `renderer`
|
|
and `camera` in particular are very likely to collide with a consuming game.
|
|
Alongside them the same file exports `akgl_mixer` and `akgl_tracks` (prefixed),
|
|
`_akgl_renderer` / `_akgl_camera` / `_akgl_physics` / `_akgl_gamemap`
|
|
(underscore-prefixed, which is reserved at file scope), and elsewhere
|
|
`HEAP_ACTOR`…`HEAP_STRING` (`src/heap.c:17-21`) and `GAME_ControlMaps`
|
|
(`src/controller.c:13`) are SCREAMING_SNAKE, which the convention reserves for
|
|
constants and macros. Settle on `akgl_` for all exported objects.
|
|
|
|
5. **`AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH`** (`include/akgl/character.h:13`) is a
|
|
character constant carrying the `SPRITE` prefix, and lives in `character.h`.
|
|
Rename to `AKGL_CHARACTER_MAX_NAME_LENGTH`.
|
|
|
|
6. **`AKGL_TIME_ONESEC_MS` is misnamed and the error is live.**
|
|
`include/akgl/game.h:22` defines it as `1000000`. One second in milliseconds
|
|
is `1000`; `1000000` is the number of nanoseconds in a *millisecond*. The name
|
|
says "one second" but the value means "one millisecond", so:
|
|
- `src/character.c:209` and `src/sprite.c:141` use it correctly as a
|
|
milliseconds-to-nanoseconds scale factor.
|
|
- `src/game.c:136` uses it as documented — a one-second budget for the state
|
|
lock — in a loop that advances `totaltime += 100` alongside `SDL_Delay(100)`.
|
|
The loop therefore runs 10,000 iterations of 100 ms, so `akgl_game_state_lock`
|
|
blocks for roughly 16 minutes rather than 1 second before reporting failure.
|
|
|
|
Rename to `AKGL_TIME_ONEMS_NS` to match `AKGL_TIME_ONESEC_NS`, and give
|
|
`akgl_game_state_lock` a real one-second budget.
|
|
|
|
### 2. Header/implementation surface drift
|
|
|
|
7. **Nineteen non-static functions are defined in `src/` but declared in no
|
|
header.** They have external linkage and public-looking names, so they are
|
|
part of the ABI whether intended or not, and no consumer can call them:
|
|
|
|
`akgl_game_load_objectnamemap`, `akgl_game_load_versioncmp`,
|
|
`akgl_game_save_actors`, `akgl_game_save_actorname_iterator`,
|
|
`akgl_game_save_charactername_iterator`, `akgl_game_save_spritename_iterator`,
|
|
`akgl_game_save_spritesheetname_iterator` (`src/game.c`);
|
|
`akgl_get_json_properties_double`, `akgl_get_json_properties_float`,
|
|
`akgl_get_json_properties_number`, `akgl_tilemap_load_layer_image`,
|
|
`akgl_tilemap_load_layer_object_actor`, `akgl_tilemap_load_physics`
|
|
(`src/tilemap.c`); `akgl_path_relative_from`, `akgl_path_relative_root`
|
|
(`src/util.c`); `gamepad_handle_added`, `gamepad_handle_button_down`,
|
|
`gamepad_handle_button_up`, `gamepad_handle_removed` (`src/controller.c`).
|
|
|
|
Each should be either declared in its header or made `static`. Note that
|
|
`tilemap.h` already has a "part of the internal API, exposed here for unit
|
|
testing" block — the tilemap entries belong there.
|
|
|
|
8. **`akgl_game_init_screen` is declared but never defined**
|
|
(`include/akgl/game.h:100`). Same failure mode as **Defects → Known and still
|
|
open #10**, which covers the four `akgl_controller_handle_*` declarations;
|
|
fold this one into that item.
|
|
|
|
9. **Static helpers use three different naming styles.** `actor_visible`
|
|
(`src/actor.c:185`) is bare; `akgl_character_load_json_inner` and
|
|
`akgl_character_load_json_state_int_from_strings` (`src/character.c:101,132`)
|
|
and `akgl_sprite_load_json_spritesheet` (`src/sprite.c:51`) carry the full
|
|
public prefix; `gamepad_handle_*` (`src/controller.c:121+`) uses a third
|
|
subsystem word that appears nowhere else. Adopt one rule — the clearest is
|
|
that `static` helpers drop the `akgl_` prefix, since it exists to avoid
|
|
external collisions.
|
|
|
|
10. **Parameter names disagree between declaration and definition.** Doxygen
|
|
documents the header spelling, so the generated docs describe names the
|
|
implementation does not use:
|
|
|
|
| Header | Implementation |
|
|
|---|---|
|
|
| `character.h:41` `basechar` | `character.c:21` `obj` |
|
|
| `character.h:69` `props` | `character.c:75` `registry` |
|
|
| `heap.h:121` `ptr` | `heap.c:143` `basechar` |
|
|
| `registry.h:97` `value` | `registry.c:163` `src` |
|
|
| `json_helpers.h:134` `e` | `json_helpers.c:149` `err` |
|
|
| `tilemap.h:134,143` `dest` | `tilemap.c:646,767` `map` |
|
|
|
|
The `tilemap.h` pair is the most misleading: the parameter is the map being
|
|
*read* and drawn, but it is named `dest` and documented as "Output
|
|
destination populated by the function".
|
|
|
|
11. **Object-pool size macros are defined twice, and the override hook is
|
|
dead.** `heap.h:15-29` wraps `AKGL_MAX_HEAP_ACTOR`, `_SPRITE`, `_SPRITESHEET`,
|
|
`_CHARACTER`, and `_STRING` in `#ifndef` guards so a consumer can override
|
|
them, but `actor.h:65`, `sprite.h:19-20`, and `character.h:14` define the same
|
|
four unconditionally and are included from `heap.h:9-11`. Whichever header
|
|
lands first wins and the `#ifndef` never fires, so the override mechanism
|
|
cannot work. Define each pool size once — `heap.h` is the natural home.
|
|
|
|
12. **Headers rely on their includers for types.** `iterator.h` uses `uint32_t`
|
|
without `<stdint.h>`; `json_helpers.h` uses `json_t` without `<jansson.h>`;
|
|
`util.h` uses `SDL_FRect` and `bool` without any SDL include; `text.h` uses
|
|
`SDL_Color` and `akerr_ErrorContext` without including SDL or `akerror.h`;
|
|
`controller.h` uses `akgl_Actor` without including `actor.h`. Each compiles
|
|
only because of `.c`-file include ordering. Headers should be self-contained.
|
|
|
|
13. **Include spelling is split between quoted and angled forms for the same
|
|
directory.** `actor.h:10-11`, `character.h:10-11`, `controller.h:11`,
|
|
`game.h:11-14`, `json_helpers.h:10`, and `staticstring.h:9` use
|
|
`#include "sibling.h"`; `physics.h:11-13`, `registry.h:9-10`, `renderer.h:13`,
|
|
`tilemap.h:10-12`, and `util.h:10` use `#include <akgl/sibling.h>`. The
|
|
angled form is correct for an installed library.
|
|
|
|
14. **Empty parameter lists.** `akgl_heap_init()`, `akgl_heap_init_actor()`,
|
|
`akgl_registry_init*()`, `akgl_game_init()`, `akgl_game_init_screen()`, and
|
|
`akgl_game_updateFPS()` declare `()` rather than `(void)`, while
|
|
`akgl_controller_list_keyboards(void)`, `akgl_controller_open_gamepads(void)`,
|
|
`akgl_game_lowfps(void)`, and `akgl_game_state_lock(void)` use `(void)`.
|
|
Before C23 the two are not equivalent — `()` suppresses argument checking.
|
|
`akgl_heap_init_actor` is even declared `()` in `heap.h:57` and defined
|
|
`(void)` in `heap.c:48`.
|
|
|
|
15. **`AKERR_NOIGNORE` is applied inconsistently at definition sites.** Headers
|
|
use it uniformly (except `akgl_sprite_sheet_coords_for_frame`, `sprite.h:86`,
|
|
which omits it). Definitions are split even within one file: `registry.c:55,120,163,172`
|
|
repeat it, `registry.c:27,44,63,71,79,96,104,112` do not. Since the attribute
|
|
is already on the declaration, drop it from all definitions.
|
|
|
|
### 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.
|
|
|
|
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.
|
|
|
|
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.
|
|
|
|
### 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.
|
|
|
|
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.
|
|
|
|
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.
|
|
|
|
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.
|
|
|
|
23. **`akgl_Frame` (`game.h:27-31`) is defined and never used** anywhere in
|
|
`src/`, `include/`, `tests/`, or `util/`.
|
|
|
|
### 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.
|
|
|
|
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`.
|
|
|
|
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.
|
|
|
|
### 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`).
|
|
|
|
28. **`point` is documented as "Represents a two-dimensional point"**
|
|
(`util.h:12`) but has `x`, `y`, and `z`.
|
|
|
|
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.
|
|
|
|
### 7. Formatting
|
|
|
|
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.
|
|
|
|
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).
|
|
|
|
**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`.
|
|
|
|
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.
|
|
|
|
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.
|
|
|
|
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).
|
|
|
|
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`.
|
|
|
|
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` 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.
|
|
|
|
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.
|
|
|
|
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.
|
|
|
|
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.
|
|
|
|
**Resolved** alongside the text measurement work; `tests/text.c` asserts a
|
|
NULL filepath reports `AKERR_NULLPOINTER`.
|
|
|
|
40. **`akgl_path_relative` and `akgl_path_relative_from` disagree on the output
|
|
parameter.** `akgl_path_relative` and `akgl_path_relative_root` take
|
|
`akgl_String *dst`; `akgl_path_relative_from` takes `akgl_String **dst`
|
|
(`src/util.c:105`). The `**` form matches the rest of the library
|
|
(`akgl_get_json_string_value`, `akgl_get_property`, `akgl_heap_next_string`),
|
|
which allocate when `*dest` is NULL. Related: **Defects → Known and still
|
|
open #4**, which covers the fact that `akgl_path_relative_from` never writes
|
|
`*dst` at all.
|
|
|
|
41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and the
|
|
`akgl_path_relative*` family use `dst`; everything else uses `dest`.
|
|
|
|
## Coverage status
|
|
|
|
Generated with:
|
|
|
|
```sh
|
|
cmake -S . -B build-coverage -DAKGL_COVERAGE=ON -DCMAKE_BUILD_TYPE=Debug
|
|
cmake --build build-coverage --parallel
|
|
ctest --test-dir build-coverage --output-on-failure
|
|
```
|
|
|
|
Reports land in `build-coverage/coverage/` (`index.html`, `coverage.xml`).
|
|
|
|
**Line coverage 79.6%, function coverage 87.2%** (2189/2749 lines, 164/188
|
|
functions), up from 77.2% / 83.1% and from a 39.6% / 44.3% baseline. All 21
|
|
suites pass. (An earlier revision of this file recorded `character` as an
|
|
intentionally failing suite; it passes now, and nothing in `tests/character.c`
|
|
or `src/character.c` has changed since — the fix came from elsewhere in the tree
|
|
and this note was never updated.)
|
|
|
|
Note the trap described in "Known and still open" item 13 while regenerating
|
|
these numbers: a coverage tree rebuilt after a source edit fails `coverage_reset`
|
|
with `GCOV returncode was 5`. `find build-coverage -name '*.gcda' -delete` clears
|
|
it.
|
|
|
|
| File | Lines | Functions |
|
|
|---|---|---|
|
|
| `src/actor.c` | 205/258 (80%) | 16/18 |
|
|
| `src/assets.c` | 0/21 (0%) | 0/1 |
|
|
| `src/audio.c` | 229/248 (92%) | 22/23 |
|
|
| `src/character.c` | 104/118 (88%) | 6/7 |
|
|
| `src/controller.c` | 300/329 (91%) | 16/16 |
|
|
| `src/draw.c` | 253/267 (95%) | 11/12 |
|
|
| `src/error.c` | 9/9 (100%) | 1/1 |
|
|
| `src/game.c` | 128/235 (54%) | 12/17 |
|
|
| `src/heap.c` | 111/111 (100%) | 12/12 |
|
|
| `src/json_helpers.c` | 111/111 (100%) | 11/11 |
|
|
| `src/physics.c` | 140/140 (100%) | 10/10 |
|
|
| `src/registry.c` | 76/102 (74%) | 11/12 |
|
|
| `src/renderer.c` | 42/75 (56%) | 7/8 |
|
|
| `src/sprite.c` | 93/101 (92%) | 5/5 |
|
|
| `src/staticstring.c` | 16/17 (94%) | 2/2 |
|
|
| `src/text.c` | 48/48 (100%) | 4/4 |
|
|
| `src/tilemap.c` | 201/426 (47%) | 10/20 |
|
|
| `src/util.c` | 121/131 (92%) | 7/8 |
|
|
| `src/version.c` | 2/2 (100%) | 1/1 |
|
|
|
|
Branch coverage reads 21.2% and should not be used as a target. The akerror
|
|
control-flow macros (`ATTEMPT`/`CATCH`/`PROCESS`/`FINISH`, `FAIL_*_RETURN`)
|
|
expand into large branch trees per call site, most of them unreachable in normal
|
|
operation — `src/game.c` reports over 1700 branches across 230 lines. Track line
|
|
and function coverage; treat branch coverage as a relative signal within a file.
|
|
|
|
### Mutation testing
|
|
|
|
`scripts/mutation_test.py` was run over the three new files as a smoke check
|
|
(`--max-mutants 8` to 10 each, so these are samples rather than exhaustive
|
|
scores):
|
|
|
|
| File | Score | Surviving mutants |
|
|
|---|---|---|
|
|
| `src/draw.c` | 90% | Deleting the `FAIL_ZERO_BREAK` on `SDL_CreateTextureFromSurface` in `akgl_draw_flood_fill` |
|
|
| `src/audio.c` | 75% | Deleting `SUCCEED_RETURN` from the static `check_voice`; deleting `spec.freq` before opening a device |
|
|
| `src/text.c` | 50% | Three in `akgl_text_rendertextat`, which had no test yet, plus one `SUCCEED_RETURN` deletion |
|
|
|
|
The first pass over `src/audio.c` scored 50% and named two real gaps, both of
|
|
which are now tested: nothing asserted that an *unconfigured* voice is audible
|
|
(the whole reason the table defaults to a square wave at full level rather than
|
|
a zeroed struct), and no envelope test used a non-zero attack *and* decay
|
|
together, so the decay measuring from the wrong origin survived. `src/draw.c`
|
|
scored 70% first and named one: the circle was only checked at its four axis
|
|
points, which a mis-signed octant reflection survives, so it now checks that
|
|
every plotted pixel has a mirror in the other three quadrants.
|
|
|
|
Re-run as a smoke check after the akbasic API work, again at 10 sampled
|
|
mutants each:
|
|
|
|
| File | Score | Surviving mutants |
|
|
|---|---|---|
|
|
| `src/controller.c` | 40% | Three `SDL_Log` deletions; `keybuffer_head`'s initialiser; `count > 0` in `keybuffer_attach_text`; the `mod` a text-only entry is given |
|
|
| `src/audio.c` | 60% | The `break` on the last `switch` case; `errctx->handled` in the device callback; `ensure_voices()` in the mixer; the mixer's voice loop bound |
|
|
|
|
Only one of those was a real gap and it is now asserted: nothing checked that
|
|
a text-only ring entry reports no modifiers. Of the rest, three are equivalent
|
|
mutants rather than misses — a ring buffer whose head starts at 1 behaves
|
|
identically, `count > 0` cannot be false where it is checked, and deleting the
|
|
`break` on a `switch`'s last case changes nothing — and the mixer's `v <=`
|
|
bound reads one voice past a zeroed table, which is undefined rather than
|
|
observable. `SDL_Log` deletions are unobservable by construction.
|
|
|
|
What is left is honestly untestable from here. Deleting a `SUCCEED_RETURN`
|
|
leaves a non-void function falling off its end, which is undefined rather than
|
|
observably wrong, and the surviving SDL branches are allocation failures the
|
|
suite has no way to provoke. The three `src/text.c` survivors in
|
|
`akgl_text_rendertextat` are gone: it is tested now, against a software
|
|
renderer, and deleting any of its three backend checks fails the suite. That
|
|
was not free — the first draft of the test used a made-up `SDL_Renderer`
|
|
pointer, which SDL refuses on its own, so the deleted-check mutant survived it.
|
|
A live renderer was what made the check observable.
|
|
|
|
### Suites
|
|
|
|
Every suite is registered through the `AKGL_TEST_SUITES` list in
|
|
`CMakeLists.txt`, which drives target creation, CTest registration, the
|
|
`WORKING_DIRECTORY`/`TIMEOUT` properties, the link line, and the
|
|
`FIXTURES_REQUIRED akgl_coverage` list together. Adding `tests/<name>.c` and the
|
|
name to that list is all a new suite needs; it can no longer be accidentally
|
|
left out of the coverage fixture.
|
|
|
|
Shared assertion helpers are in `tests/testutil.h`: `TEST_ASSERT`,
|
|
`TEST_ASSERT_FEQ`, `TEST_EXPECT_STATUS`, `TEST_EXPECT_OK`, `TEST_EXPECT_ANY_ERROR`,
|
|
and `TEST_ASSERT_FLAG`. All except the last expand to a `break` on failure, so
|
|
they belong directly inside an `ATTEMPT` block, not inside a loop nested in one.
|
|
|
|
Done:
|
|
|
|
- `tests/physics.c` — both backends, the factory, and the full simulation loop
|
|
including thrust clamping, drag, layer masking, parent/child positioning, and
|
|
logic-interrupt handling. 100%.
|
|
- `tests/heap.c` — pool exhaustion for all five pools, refcount clamping,
|
|
recursive child release, registry cleanup on release. 100%.
|
|
- `tests/json_helpers.c` — every typed accessor, both string accessors'
|
|
allocate-vs-reuse paths, array bounds, and `akgl_get_json_with_default`. 100%.
|
|
- `tests/controller.c` — control map push and capacity, the default binding set,
|
|
keyboard and gamepad dispatch including cross-device rejection, the dpad
|
|
handlers, and device add/remove against the dummy drivers. 90%.
|
|
- `tests/game.c` — version gating, the save/load roundtrip, foreign-save
|
|
rejection, truncated-table detection, the state lock, and FPS accounting. 54%.
|
|
- `tests/actor.c` — extended with the eight control-map handlers, automatic
|
|
facing, movement logic, the animation frame state machine, `akgl_actor_update`,
|
|
and character/sprite binding lookups. 80%.
|
|
- `tests/audio.c` — every waveform, the ADSR envelope stage by stage, gate
|
|
expiry and release, the frequency sweep frame by frame, voice summing and
|
|
clamping, the master level, and device open/shutdown under the dummy driver.
|
|
92%.
|
|
- `tests/draw.c` — every primitive against a 64x64 software renderer with the
|
|
pixels read back, including flood-fill containment, save/paste roundtrip, and
|
|
that drawing restores the renderer's draw color. 95%.
|
|
- `tests/text.c` — font loading into the registry, both measurement entry points
|
|
against a monospaced fixture font, and drawing through a bound backend over a
|
|
software renderer, including every way the backend can be unusable. 100%.
|
|
- `tests/renderer.c` — the 2D vtable binding, frame start and end, both
|
|
`draw_texture` paths, the refusals a bound-but-unrendered backend gives, and
|
|
the `draw_mesh` stub. 56%; `akgl_render_init2d` and `akgl_render_2d_draw_world`
|
|
are what is left, and both want the harness.
|
|
- `tests/headers.c` — that `akgl/controller.h` compiles as the first include in
|
|
a translation unit. The assertion is the compile; `main()` only has to return
|
|
zero.
|
|
|
|
## Remaining work
|
|
|
|
### Needs the offscreen renderer harness
|
|
|
|
`akgl_render_init2d` and `akgl_render_2d_draw_world` in `src/renderer.c` (33
|
|
lines), `akgl_draw_background` in `src/draw.c` (13), `src/assets.c` (21),
|
|
`akgl_actor_render`/`actor_visible` in `src/actor.c` (53), and the drawing half
|
|
of `src/tilemap.c` all need a live `renderer` global, a window, or the world
|
|
globals.
|
|
|
|
`akgl_text_rendertextat` was on this list and is not any more: `tests/text.c`
|
|
builds a software renderer and binds a backend to it with `akgl_render_bind2d`
|
|
in nine lines, which is enough for anything that only needs *a* renderer rather
|
|
than the whole world. `tests/renderer.c` and `tests/draw.c` do the same. The
|
|
harness is still wanted, but for what is left it is a convenience rather than
|
|
the blocker it was.
|
|
|
|
Build `tests/harness.c` / `tests/harness.h` with `akgl_test_init_headless()` and
|
|
`akgl_test_shutdown_headless()`: set the dummy video and audio drivers,
|
|
`SDL_Init()`, `akgl_heap_init()`, `akgl_registry_init()`, create a software
|
|
`SDL_CreateWindowAndRenderer`, and point the global `renderer` at it. Seven
|
|
existing tests hand-roll this today (`tests/sprite.c:194`, `tests/character.c:200`,
|
|
`tests/tilemap.c:421`, `tests/charviewer.c:42`, plus `tests/draw.c`,
|
|
`tests/renderer.c` and `tests/text.c`); collapse them onto the shared harness in
|
|
the same change.
|
|
|
|
Then:
|
|
|
|
- **`tests/renderer.c` extensions** — `akgl_render_init2d` populating the camera
|
|
from the property registry, and `draw_world` layer ordering. `tests/renderer.c`
|
|
exists and covers everything that does not need a world or the registry: the
|
|
vtable binding, frame start/end against a NULL `sdl_renderer`, both
|
|
`draw_texture` paths including `angle != 0` with a NULL center, and the
|
|
`draw_mesh` stub. Note `defflags` at `src/renderer.c:113` is uninitialized
|
|
until the `if` body runs.
|
|
- **`tests/assets.c`** — BGM loading into `AKGL_REGISTRY_MUSIC` under the dummy
|
|
audio driver.
|
|
- **`tests/draw.c` extensions** — `akgl_draw_background` at zero, negative and
|
|
oversized dimensions. `tests/draw.c` exists and covers every other primitive
|
|
against a software renderer; `akgl_draw_background` is the one function in the
|
|
file that still reads the global `renderer` rather than taking a backend.
|
|
- **`tests/tilemap.c` extensions** — `akgl_tilemap_draw`, `_draw_tileset`, and
|
|
`akgl_tilemap_load_layer_image`.
|
|
|
|
### Does not need a renderer
|
|
|
|
- **`src/tilemap.c`** — `akgl_tilemap_scale_actor` is pure math over three
|
|
branches (`src/tilemap.c:824-830`); `akgl_get_json_properties_number`,
|
|
`_float`, and `_double` need only a JSON snippet; `akgl_tilemap_load_physics`
|
|
needs a fixture with the physics property block present, absent, and
|
|
malformed. Together roughly 100 of the 225 uncovered lines.
|
|
- **`src/game.c`** — `akgl_game_init`, `akgl_game_update`, `akgl_game_lowfps`,
|
|
and `akgl_game_updateFPS`'s frame loop need a window; revisit after the
|
|
harness lands.
|
|
- **`src/registry.c`** — `akgl_registry_load_properties` needs a fixture with a
|
|
`properties` object, plus the missing-file, missing-key, and wrong-value-type
|
|
cases. Assert the loop at `src/registry.c:148-158` does not leak the string
|
|
heap.
|
|
|
|
## Defects
|
|
|
|
### Fixed while building the suites
|
|
|
|
Each was found by a test written to assert correct behavior.
|
|
|
|
1. **`akgl_physics_simulate` dereferenced `self` before its NULL check.**
|
|
`src/physics.c:132` read `self->gravity_time` at declaration time, three
|
|
lines above `FAIL_ZERO_RETURN(e, self, ...)`. A NULL backend segfaulted.
|
|
2. **`akgl_game_save` never flushed or closed its stream.** `CLEANUP` and
|
|
`PROCESS` were transposed, which put the `fclose` inside the `PROCESS`
|
|
switch, where it only ran if an error context existed and reported success.
|
|
An ordinary save produced an empty file.
|
|
3. **`akgl_game_save_actors` wrote name-table terminators from a single char.**
|
|
`aksl_fwrite((void *)&nullval, 1, AKGL_ACTOR_MAX_NAME_LENGTH, fp)` emitted
|
|
127 bytes of adjacent stack memory into the save file and produced a sentinel
|
|
the loader could not recognize.
|
|
4. **`akgl_game_load_objectnamemap` swallowed read failures.** `CATCH` used
|
|
directly inside `while (1)` breaks the loop, not the function, so a truncated
|
|
or corrupt name table loaded as a successful game.
|
|
5. **`akgl_Actor_cmhf_up_on` and `_down_on` dereferenced `basechar` unguarded**,
|
|
unlike their left and right counterparts.
|
|
6. **`akgl_actor_logic_movement` checked `actor` twice** instead of checking
|
|
`actor->basechar` before dereferencing it.
|
|
7. **The gamepad handlers checked `appstate` three times each**, so a NULL event
|
|
or a missing player actor was never caught and `player->state` was
|
|
dereferenced regardless.
|
|
|
|
### 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.
|
|
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.
|
|
3. **`akgl_registry_init` never initializes the properties registry.**
|
|
`akgl_registry_init_properties()` is not called from `akgl_registry_init()`
|
|
(`src/registry.c:27`), so `AKGL_REGISTRY_PROPERTIES` stays 0 unless the
|
|
caller initializes it separately. `akgl_set_property` is then a silent no-op
|
|
and `akgl_get_property` always returns the caller's default, which means
|
|
`akgl_physics_init_arcade` and `akgl_render_init2d` silently ignore
|
|
configuration. `akgl_game_init` does call it; a caller that does not use
|
|
`akgl_game_init` does not get it.
|
|
4. **`akgl_path_relative_from` is a stub that leaks.** `src/util.c:105` claims a
|
|
heap string, never writes `*dst`, and never releases it. 256 calls exhaust
|
|
the string pool.
|
|
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.
|
|
6. **`akgl_string_initialize` overflows by four bytes when `init` is NULL.**
|
|
`src/staticstring.c:17` does `memset(&obj->data, 0x00, sizeof(akgl_String))`,
|
|
but `data` starts four bytes into the struct (after `refcount`), so the memset
|
|
runs four bytes past the end.
|
|
7. **Savegame name lengths disagree between writer and reader.**
|
|
`akgl_game_save_actors` writes spritesheet names at
|
|
`AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` (512) and character names at
|
|
`AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH` (128), but `akgl_game_load` reads
|
|
every table at `AKGL_ACTOR_MAX_NAME_LENGTH` (128). A save with any registered
|
|
spritesheet cannot be read back. The current roundtrip test passes only
|
|
because empty registries write nothing but zeroed sentinels.
|
|
8. **Heap acquire functions are asymmetric.** `akgl_heap_next_string` increments
|
|
`refcount`; `next_actor`, `next_sprite`, `next_spritesheet`, and
|
|
`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.**
|
|
10. **`controller.h` declares functions that do not exist.** It declares
|
|
`akgl_controller_handle_button_down`, `_button_up`, `_added`, and `_removed`,
|
|
but `src/controller.c` defines them as `gamepad_handle_*`. Anything compiled
|
|
against the header alone fails to link.
|
|
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`.
|
|
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`
|
|
has no `set -e` and never checks `curl`'s exit status: on a failed fetch it
|
|
writes `mappings.txt` empty, then overwrites the good tracked header with
|
|
`AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty initializer — and exits 0.
|
|
Verified by pointing the script at an unresolvable host.
|
|
|
|
Two compounding problems:
|
|
|
|
- `add_custom_command` at `CMakeLists.txt:89-93` declares
|
|
`OUTPUT include/akgl/SDL_GameControllerDB.h` as a *relative* path, which
|
|
CMake resolves against the binary directory, while the script writes to
|
|
the source directory. The declared output never appears, so the command is
|
|
permanently out of date and re-runs on every build — making every build
|
|
depend on network access and leaving the file dirty in the working tree
|
|
each time.
|
|
- `const char *SDL_GAMECONTROLLER_DB[] = {\n};` is an empty initializer,
|
|
which is a constraint violation in ISO C and compiles only as a GCC
|
|
extension.
|
|
|
|
Fix: have the script fetch to a temporary file, check `curl`'s status and a
|
|
plausible minimum line count, and leave the existing header untouched on
|
|
failure (exiting non-zero). Separately, make regeneration explicit — a
|
|
dedicated `controllerdb` target, or an `OUTPUT` that matches where the
|
|
script actually writes — so an ordinary build neither needs the network nor
|
|
dirties the tree.
|
|
13. **A stale build tree in the source directory breaks the coverage run.**
|
|
`CMakeLists.txt:208` and `CMakeLists.txt:223` pass
|
|
`--root "${CMAKE_CURRENT_SOURCE_DIR}"` to gcovr, and gcovr searches for
|
|
`.gcda`/`.gcno` files under the root. The `--object-directory` argument on
|
|
the line below each does *not* narrow that search: per `gcovr --help` it
|
|
only identifies "the path between gcda files and the directory where the
|
|
compiler was originally run". So every instrumented build tree left inside
|
|
the source directory is folded into the report alongside the one actually
|
|
being measured.
|
|
|
|
With a `build-coverage/` from an earlier session still present, a freshly
|
|
configured `-DAKGL_COVERAGE=ON` tree fails in `coverage_reset`, before any
|
|
test runs:
|
|
|
|
AssertionError: Got function akgl_game_lowfps on multiple lines: 45, 46.
|
|
|
|
45 and 46 are that function's line numbers before and after an unrelated
|
|
`#include` was added to `src/game.c`: gcovr found 18 stale `.gcno` files
|
|
describing the old layout, merged them with the current ones, and could not
|
|
reconcile the two. Moving `build-coverage/` aside makes the same tree pass
|
|
18/18. Verified with gcovr 7.0.
|
|
|
|
The `build*/` entry in `.gitignore` hides these trees from `git status`,
|
|
which makes the state easier to get into and no easier to notice.
|
|
|
|
A second, smaller version of the same thing: rebuilding an existing
|
|
coverage tree after editing a test leaves `.gcda` files describing the old
|
|
object layout, and `coverage_reset` -- whose whole job is to delete them --
|
|
fails with `GCOV returncode was 5` before it gets the chance. `find
|
|
build-coverage -name '*.gcda' -delete` clears it. Observed with gcovr 7.0.
|
|
|
|
Fix: pass the build tree to gcovr as an explicit search path instead of
|
|
letting it default to `--root`, so only the tree under measurement is
|
|
considered. Touches the two `add_test` blocks at `CMakeLists.txt:204-211`
|
|
and `CMakeLists.txt:220-229`; nothing outside the `AKGL_COVERAGE` branch.
|
|
|
|
14. **22 public symbols shipped without a version or soname bump.** 42b60f7 added
|
|
`akgl_draw_point`, `_line`, `_rect`, `_filled_rect`, `_circle`, `_flood_fill`,
|
|
`_copy_region` and `_paste_region`; `akgl_audio_init`, `_shutdown`, `_tone`, `_stop`,
|
|
`_waveform`, `_envelope`, `_volume`, `_voice_active` and `_mix`;
|
|
`akgl_controller_poll_key` and `_flush_keys`; and `akgl_text_measure` and
|
|
`_measure_wrapped`. `project(akgl VERSION 0.1.0)` and the `libakgl.so.0.1` soname were
|
|
both left alone.
|
|
|
|
That contradicts this repository's own stated rule, which `akbasic`'s `CLAUDE.md` quotes
|
|
back: *"for both 0.x libraries the soname carries `MAJOR.MINOR` deliberately: 0.1 and 0.2
|
|
are different ABIs."* Adding exported symbols under an unchanged soname has two
|
|
consequences, and the first is the one that bites:
|
|
|
|
- A binary compiled against the new headers links happily against a `libakgl.so.0.1`
|
|
built from the old tree, because the soname says they are the same ABI. It fails at
|
|
symbol resolution rather than at configure time.
|
|
- `AKGL_VERSION_AT_LEAST(0, 1, 0)` is true for both trees, so a consumer cannot
|
|
feature-test for the new API at all. `akbasic` pins the requirement by submodule commit
|
|
instead, and says so in its README, which is not a thing a released library should make
|
|
anybody do.
|
|
|
|
Fix: bump `project()` to 0.2.0, which carries the soname to `libakgl.so.0.2` through the
|
|
existing logic at `CMakeLists.txt:138`. `akstdlibConfigVersion.cmake`'s `SameMinorVersion`
|
|
equivalent then refuses the mismatch at configure time as well.
|
|
|
|
**Resolved by 1066ac7**, which did exactly that while this was being written — the two
|
|
crossed, rather than one following the other. Kept rather than deleted because the *reason*
|
|
is still the useful part: `akbasic` could not feature-test for the new API and had to pin
|
|
`libakgl` by submodule commit in its README until this landed, which is the concrete cost of
|
|
an additive release under an unchanged soname. Worth remembering the next time a handful of
|
|
symbols looks too small to bump for.
|
|
|
|
### Found while rewriting the Doxygen comments
|
|
|
|
Each of these came out of reading an implementation against the contract its
|
|
header claimed. They are recorded inline as `@note` or `@warning` on the
|
|
function concerned, so a reader of the generated documentation finds them
|
|
without coming here first. Ordered by blast radius.
|
|
|
|
15. **`akgl_path_relative` leaks one error-context slot per call on its fallback
|
|
path.** `src/util.c:120` returns `akgl_path_relative_root(...)` from inside
|
|
the `HANDLE(e, ENOENT)` block. `FINISH` is what carries `RELEASE_ERROR`, so
|
|
returning before it never gives the context back. libakerror hands these out
|
|
of a fixed `AKERR_ARRAY_ERROR[128]`, and this is not a rare path — it is the
|
|
*ordinary* one, taken every time an asset names a neighbour relative to its
|
|
own file rather than to the working directory. Every tileset image, layer
|
|
image and spritesheet in a map costs a slot, permanently. Once the array is
|
|
exhausted every subsequent failure anywhere in the process has nowhere to
|
|
report from.
|
|
|
|
Fix: assign the result to a local, `break`, and return after `FINISH`; or
|
|
hoist the fallback out of the handler entirely and let the `HANDLE` block
|
|
only record that a retry is wanted. Touches `src/util.c:117-121` only.
|
|
|
|
16. **`akgl_sprite_load_json` does not bound the `frames` array.**
|
|
`src/sprite.c:165` sets `obj->frames` from `json_array_size()` and
|
|
`src/sprite.c:167` then writes that many entries into
|
|
`frameids[AKGL_SPRITE_MAX_FRAMES]`, which is 16. A sprite definition with 17
|
|
or more frames writes past the array into the rest of `akgl_Sprite`, and
|
|
past the struct into the neighbouring pool slot. It is also written through
|
|
a `uint32_t *` cast of a `uint8_t *`, so each element write touches four
|
|
bytes; that happens to work on a little-endian machine because the following
|
|
iterations overwrite the spill and the last write lands in the struct's
|
|
alignment padding, but it is not portable and it is what makes the overflow
|
|
reach four bytes past `frames` rather than one.
|
|
|
|
Fix: refuse a `frames` array longer than `AKGL_SPRITE_MAX_FRAMES` with
|
|
`AKERR_OUTOFBOUNDS`, and read into an `int` local before narrowing to
|
|
`uint8_t`. Touches `src/sprite.c:164-169`.
|
|
|
|
17. **Two more unbounded array loads in the tilemap loader.**
|
|
`src/tilemap.c:387-389` walks a Tiled object layer straight into
|
|
`curlayer->objects[j]` with no check against
|
|
`AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER` (128), and `src/tilemap.c:278-280`
|
|
fills `dest->tilesets[i]` with no check against `AKGL_TILEMAP_MAX_TILESETS`
|
|
(16).
|
|
Both are the same shape as item 16 and both are reachable from an ordinary
|
|
map file: 128 objects is not a large object layer. Note that
|
|
`akgl_tilemap_load_layers` *does* bound its loop and raises
|
|
`AKERR_OUTOFBOUNDS`, so the pattern to copy is already in the same file.
|
|
|
|
Fix: add the bound check at the top of each loop body, matching
|
|
`akgl_tilemap_load_layers`. Touches `src/tilemap.c:387` and
|
|
`src/tilemap.c:278`.
|
|
|
|
18. **`akgl_get_json_with_default` defaults on a status the array accessors never
|
|
raise.** `src/json_helpers.c:164-165` handles `AKERR_KEY` and `AKERR_INDEX`,
|
|
but `akgl_get_json_array_index_object`, `_integer` and `_string` all report
|
|
a short array as `AKERR_OUTOFBOUNDS`. So the "this element is optional" form
|
|
silently does not work for an array — the error propagates instead of being
|
|
replaced by the default. Only the object accessors, which do raise
|
|
`AKERR_KEY`, are actually served by this function today. Nothing in tree
|
|
passes an array accessor's error here, which is why it has not been noticed.
|
|
|
|
Fix: add a third `HANDLE_GROUP(err, AKERR_OUTOFBOUNDS)`. Note that the
|
|
existing two rely on `HANDLE_GROUP` not emitting a `break`, so the `KEY`
|
|
case falls through into the `INDEX` body — a third arm has to go *above* the
|
|
one holding the `memcpy`, not below it. Touches `src/json_helpers.c:164-167`.
|
|
|
|
19. **The background music never loops.** `src/assets.c:20` initialises
|
|
`bgmprops` to 0, `src/assets.c:44` sets `MIX_PROP_PLAY_LOOPS_NUMBER` on it,
|
|
and `src/assets.c:46` plays the track with it. 0 is SDL's "no property set"
|
|
sentinel, not a set this function owns, so the write is rejected —
|
|
unchecked — and the play call is given no options. The music plays once and
|
|
stops. `akgl_load_start_bgm` reports success either way.
|
|
|
|
Fix: `SDL_CreateProperties()` into `bgmprops`, check it, and destroy it in
|
|
`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.
|
|
|
|
Fix: read the existing entry first and `akgl_heap_release_sprite` it, and
|
|
check `SDL_SetPointerProperty`'s return. Touches `src/character.c:43-55`.
|
|
|
|
21. **`akgl_heap_release_character` abandons the whole state-to-sprite map.**
|
|
`src/heap.c:150` zeroes the character without walking `state_sprites`, so
|
|
every sprite reference the character took in `akgl_character_sprite_add` is
|
|
lost and the `SDL_PropertiesID` holding the map is never destroyed. Loading
|
|
and releasing characters in a loop — level to level — exhausts the sprite
|
|
pool and leaks an SDL property set each time.
|
|
`akgl_character_state_sprites_iterate` with `AKGL_ITERATOR_OP_RELEASE` exists
|
|
precisely to do this walk and is not called from here.
|
|
|
|
Fix: enumerate `state_sprites` with that iterator, then
|
|
`SDL_DestroyProperties`, before the `memset`. Touches `src/heap.c:145-151`.
|
|
|
|
22. **`akgl_path_relative_root` uses `FAIL_RETURN` inside its `ATTEMPT` block.**
|
|
`src/util.c:75` returns past `CLEANUP` on the over-long-path branch, so the
|
|
two scratch strings claimed at `src/util.c:67-68` are never released. This is
|
|
the exact hazard AGENTS.md documents under the error-handling protocol, and
|
|
the same class as the heap-string leak already fixed in
|
|
`akgl_get_json_tilemap_property`. Smaller blast radius than item 15 because
|
|
the branch is rare, but it is a mechanical fix.
|
|
|
|
Fix: `FAIL_BREAK`. Touches `src/util.c:75`.
|
|
|
|
23. **Three smaller leaks on failure paths.** All the same shape: a resource
|
|
acquired before an `ATTEMPT`/`CLEANUP` pair, or released only on the success
|
|
path.
|
|
|
|
- `src/renderer.c:26-32` — `akgl_render_init2d` releases the two pooled
|
|
strings holding the screen dimensions only after both `aksl_atoi` calls
|
|
succeed, so a non-numeric `game.screenwidth` leaks two string slots.
|
|
- `src/controller.c:80` — `akgl_controller_open_gamepads` calls
|
|
`SDL_free(gamepads)` only after the loop completes, so a gamepad that
|
|
fails to open leaks the enumeration array.
|
|
- `src/text.c:57-64` — `akgl_text_rendertextat` destroys the surface and
|
|
texture only on the success path, so a failed texture upload or a failed
|
|
draw leaks both. On a HUD line redrawn every frame that is a leak per
|
|
frame.
|
|
|
|
24. **`akgl_get_property` reads past the end of the property value.**
|
|
`src/registry.c:182` copies a fixed `AKGL_MAX_STRING_LENGTH` bytes out of
|
|
whatever `SDL_GetStringProperty` returned, rather than that string's length.
|
|
The destination is a full-sized pool string so nothing is corrupted, but the
|
|
source is an ordinary NUL-terminated string owned by SDL and the read runs
|
|
up to PATH_MAX bytes past its end. Benign in practice and immediately fatal
|
|
under ASAN, which is the reason to fix it rather than leave it.
|
|
|
|
Fix: `aksl_strlcpy` or an explicit length. Touches `src/registry.c:181-186`.
|
|
|
|
25. **`akgl_character_load_json_state_int_from_strings` checks the same argument
|
|
twice.** `src/character.c:123` guards `states` and `src/character.c:124`
|
|
guards `states` again under the message "NULL destination integer" — the
|
|
guard for `dest` was never written. A `NULL` `dest` is dereferenced at
|
|
`src/character.c:132`. Only reachable from inside the character loader, which
|
|
always passes a real pointer, so it is a latent hole rather than a live bug.
|
|
|
|
Fix: change the second guard's subject to `dest`. Touches
|
|
`src/character.c:124`.
|
|
|
|
26. **`akgl_actor_render` computes a sprite's drawn height from its width.**
|
|
`src/actor.c:276` sets `dest.h = curSprite->width * obj->scale` where it
|
|
should read `curSprite->height`. Every actor is therefore drawn square, and
|
|
a non-square sprite is stretched or squashed. Invisible in the current
|
|
assets because they are square; it will not stay that way.
|
|
|
|
Fix: one word. Touches `src/actor.c:276`. Worth a test that renders a
|
|
deliberately non-square sprite and asserts on the destination rectangle.
|
|
|
|
### Found while closing the akbasic API gaps
|
|
|
|
27. **`akgl_text_rendertextat` refuses the empty string, and `akgl_text_measure` accepts it.**
|
|
SDL_ttf returns `NULL` with "Text has zero width" from both `TTF_RenderText_Blended` and
|
|
`TTF_RenderText_Blended_Wrapped` for `""`, so `src/text.c:56`'s `FAIL_ZERO_RETURN` on the
|
|
surface reports `AKERR_NULLPOINTER` for what a caller means as "draw nothing". Verified
|
|
directly against SDL_ttf 3.x with the fixture font.
|
|
|
|
`akgl_text_measure("")` is documented as legal and returns 0 wide by one line high, which
|
|
is what a cursor sitting on an empty line needs, so the two halves of the same header
|
|
disagree about the same string. The functional consequence is a caller that draws a line of
|
|
text which may be empty — a line editor, a `PRINT` of an empty string, a HUD field that has
|
|
not been filled in yet — has to test for it before every call or handle a failure that
|
|
means nothing went wrong.
|
|
|
|
**Fix:** return success without rasterizing when `text[0] == '\0'`, matching the measure
|
|
side, and say so in the header. Touches `akgl_text_rendertextat` only. `tests/text.c` has
|
|
the case written and deliberately not asserted; it would become a `TEST_EXPECT_OK`. Until
|
|
then the header carries the wart as a `@note` pointing here.
|
|
|
|
## Performance
|
|
|
|
The first measured baseline is in `PERFORMANCE.md`, produced by `tests/perf.c`
|
|
and `tests/perf_render.c` (`ctest --test-dir build -L perf`). Both suites hold
|
|
every measurement to a budget set at roughly ten times the recorded baseline, so
|
|
an algorithmic regression fails the suite rather than being discovered by a
|
|
player. Read `PERFORMANCE.md` before arguing with anything below — every claim
|
|
here has a number behind it, and several of the things I expected to be slow are
|
|
not.
|
|
|
|
### Defects the perf suites found
|
|
|
|
Ordered by blast radius. Numbering continues the **Defects** list above.
|
|
|
|
28. **`akgl_path_relative` leaked an error context on every root-fallback
|
|
resolution.** `src/util.c:118` took its `ENOENT` branch by `return`ing from
|
|
inside the `HANDLE` block, which skips the `RELEASE_ERROR` that `FINISH`
|
|
ends with. One entry of `AKERR_ARRAY_ERROR` was lost per call, and the 129th
|
|
call hit "Unable to pull an error context from the array!" and **exited the
|
|
process**. Every tilemap load resolves several paths this way, so a game
|
|
that loaded fifty levels died in the loader.
|
|
|
|
**Fixed.** The branch now records a flag and calls
|
|
`akgl_path_relative_root` after `FINISH`. `tests/util.c` carries
|
|
`test_akgl_path_relative_releases_contexts`, which resolves
|
|
`AKERR_MAX_ARRAY_ERROR * 2` paths through that branch and asserts the pool
|
|
is where it started; against the old code that test does not fail, it
|
|
terminates the suite.
|
|
|
|
This is the *only* `return` from inside a `HANDLE` block in `src/` — the
|
|
other candidates use `SUCCEED_RETURN`, which releases correctly. Worth a
|
|
grep before anyone writes a new one, and worth a line in AGENTS.md's
|
|
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.
|
|
|
|
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.
|
|
|
|
Until it is fixed, the tilemap-load benchmark in `tests/perf_render.c`
|
|
reclaims the pool by hand between iterations, and says so.
|
|
|
|
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.
|
|
|
|
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.
|
|
|
|
31. **`akgl_game_update` segfaults if `akgl_game_init` did not run.**
|
|
`src/game.c:182` calls `game.lowfpsfunc()` through the pointer with no NULL
|
|
check, on every frame where `game.fps` is under 30 — which includes the
|
|
first frame, before there is a frame rate to compare against. `akgl_game_init`
|
|
installs the default; nothing else does.
|
|
|
|
That matters because `include/akgl/renderer.h` documents the other path on
|
|
purpose: a host that owns its own window and calls `akgl_render_bind2d`
|
|
instead of `akgl_render_init2d`. An embedder following that documentation
|
|
and then calling `akgl_game_update` crashes on frame one. `akbasic` is
|
|
exactly that embedder.
|
|
|
|
Fix: guard the call, or have `akgl_game_updateFPS` install the default when
|
|
it finds a NULL. `tests/perf_render.c` installs it by hand as a workaround
|
|
and points here.
|
|
|
|
32. **`akgl_game_update` runs the actor update sweep once per tilemap layer.**
|
|
`src/game.c:617` loops `i` over `AKGL_TILEMAP_MAX_LAYERS` and the actor
|
|
sweep nested inside it never compares `actor->layer` to `i`, so every live
|
|
actor's `updatefunc` runs **16 times per frame**. Measured: 68.4 ns per
|
|
update, 64 actors, so 70 µs of work to do 4.4 µs of work.
|
|
|
|
It hides behind the rasterizer today (a 640x480 software frame is 16 ms) and
|
|
it will not hide behind a GPU backend, where a frame is nearer 2 ms and this
|
|
is 3.5% of it. Fix: either filter by layer like `akgl_render_2d_draw_world`
|
|
does, or hoist the update sweep out of the layer loop entirely — updating an
|
|
actor is not a per-layer operation. The second is almost certainly right,
|
|
and it is the one that needs a test asserting `updatefunc` runs exactly once
|
|
per actor per `akgl_game_update`.
|
|
|
|
33. **`akgl_heap_release_character` leaks its `state_sprites` property set.**
|
|
Already documented in `heap.h` and in **Carried over** item 1; the perf
|
|
suite makes it measurable rather than theoretical. A benchmark that loads
|
|
the fixture character 10,000 times leaks 10,000 SDL property sets and 20,000
|
|
sprite references. Cross-referenced here only because the character-load
|
|
benchmark had to be written around it.
|
|
|
|
### Targets
|
|
|
|
What a library like this *should* hit. These are not predictions of what the
|
|
current code does — several are missed today, and each says which. The frame
|
|
budget throughout is 16.67 ms (60 fps); where a target is stated per-operation
|
|
it is because that is the number that survives a change of renderer.
|
|
|
|
The one that governs the rest: **libakgl's own bookkeeping should never be the
|
|
reason a frame is late.** Everything the library decides — pool scans, registry
|
|
lookups, state-to-sprite mapping, physics, visibility, error contexts — should
|
|
fit in 5% of a frame, leaving 95% for the pixels and the game's own logic. At
|
|
64 actors and a screenful of tiles that is a ceiling of about 800 µs.
|
|
|
|
| # | Target | Today | Verdict |
|
|
|---|---|---|---|
|
|
| 1 | Library bookkeeping under 5% of a 60 fps frame at 64 actors + 1200 tiles | ~0.3% (excluding pixel work) | **met** |
|
|
| 2 | One actor's logic update under 200 ns | 68.4 ns | **met** |
|
|
| 3 | One actor's render bookkeeping, excluding the blit, under 500 ns | ~250 ns, by subtraction rather than direct measurement | **met, weakly measured** |
|
|
| 4 | Physics sweep under 25 ns per *live* actor, and proportional to live actors rather than pool size | 19 ns per live actor, but 63.9 ns for an *empty* pool | **partly** |
|
|
| 5 | Tilemap draw bookkeeping under 100 ns per tile, excluding the blit | ~25 ns | **met** |
|
|
| 6 | Pool acquire under 100 ns regardless of how full the pool is | 3.9 ns empty, 250.9 ns on the last free string slot | **missed** |
|
|
| 7 | Pool release proportional to the bytes actually used, not to the slot's capacity | 47.2 ns, a fixed 4 KiB wipe | **missed** |
|
|
| 8 | Re-drawing an unchanged line of text under 1 µs | 12.6 µs, every frame, no cache | **missed** |
|
|
| 9 | Zero texture creation or destruction per frame in steady state | one create + one destroy per line of text per frame | **missed** |
|
|
| 10 | A handled, routine condition costs no more than twice the path that succeeds | 616.5 ns vs 68.4 ns — 9x | **missed** |
|
|
| 11 | 256 actors simulated, updated and made ready to draw in under 1 ms | ~22 µs at 64 actors (5.8 µs measured logic + estimated render bookkeeping); linear, so ~90 µs extrapolated | **met, untested at that size** |
|
|
| 12 | Collision for 256 actors under 2 ms without the caller writing a broad phase | no broad phase exists; the naive loop is 1.9 ms at 256 actors | **missed** |
|
|
| 13 | Level load under 100 ms for a map with 8 tilesets, 4 layers and 64 actors | 11.9 ms for a 2x2 map with one tileset | **unknown at that size** |
|
|
| 14 | Fixed per-load overhead under 1% of a level load | 11.5% — zeroing 26 MB of `akgl_Tilemap` | **missed** |
|
|
| 15 | Static footprint under 4 MB in the default configuration | 28 MB, 94% of it one tilemap | **missed** |
|
|
| 16 | No pool leaks across a load/release cycle of any asset type | tilemap leaks 5 strings per cycle (item 29) | **missed** |
|
|
| 17 | Pool exhaustion reports `AKGL_ERR_HEAP` and never crashes | item 30 crashes | **missed** |
|
|
| 18 | Every benchmark held to 10x its recorded baseline, enforced in CI | done, `ctest -L perf` | **met** |
|
|
|
|
Notes on the ones worth arguing about:
|
|
|
|
- **6 and 7 are the same fix.** The acquire scan is 64x slower on a full string
|
|
pool than an empty one purely because the pool is a megabyte and the scan
|
|
touches one refcount per 4 KiB. A free-list index — one `int` per layer,
|
|
remembering where the last free slot was — takes both to constant time without
|
|
changing the "no `malloc`" rule at all. That is the change I would make first,
|
|
and it is worth doing *before* anyone raises `AKGL_MAX_HEAP_*`, because the
|
|
cost of the current design grows with the ceiling rather than with the usage.
|
|
|
|
- **8 and 9 are one cache.** A single entry keyed on (font, string, colour,
|
|
wrap) would cover the common case — a HUD field that changes once a second —
|
|
and a four- or eight-entry ring would cover the rest. This is the clearest
|
|
optimisation in the library and it is maybe forty lines.
|
|
|
|
- **10 is a design target, not a speed target.** The answer is not a faster
|
|
error context. It is that "this character has no sprite for this state" is a
|
|
question with a boolean answer, and reporting it through `AKERR_KEY` costs
|
|
nine times the update it replaces. `akgl_character_sprite_get` wants a
|
|
companion that returns `NULL` without raising.
|
|
|
|
- **12 is a scope decision, not a defect.** The library deliberately does not
|
|
own a broad phase, and at 64 actors the naive all-pairs loop is 0.7% of a
|
|
frame — genuinely fine. At 256 it is 11%. Either the ceiling stays where it is
|
|
and this target is dropped, or a uniform grid keyed on tile size goes in. I do
|
|
not think a spatial index belongs here yet; I think the target belongs on
|
|
record so that raising `AKGL_MAX_HEAP_ACTOR` is a decision made with the
|
|
number in front of it.
|
|
|
|
- **14 and 15 are the same fix too.** `akgl_Tilemap` is 26 MB because every
|
|
layer carries a 512x512 `int` grid and every tileset a 65,536-entry offset
|
|
table, sized for the worst case at compile time. The pool rule does not
|
|
require *this*: a layer could carry an index into one shared cell arena sized
|
|
by `AKGL_TILEMAP_MAX_WIDTH * AKGL_TILEMAP_MAX_HEIGHT` once rather than
|
|
sixteen times, and the offset table could be sized by `tilecount` rather than
|
|
by the maximum. That is a real refactor with a real ABI break, so it belongs
|
|
to 0.4 rather than to a patch release — but 28 MB of BSS on a handheld or an
|
|
ESP32-class target is the difference between fitting and not.
|
|
|
|
- **13 is untested and should not stay that way.** The only map fixture in the
|
|
tree is 2x2 with one tileset, which is why the load benchmark measures a PNG
|
|
decode and a `memset` rather than a map. A realistic fixture — 128x128, four
|
|
layers, several tilesets — would make target 13 measurable and would probably
|
|
find something.
|
|
|
|
## Memory checking
|
|
|
|
`cmake --build build --target memcheck` runs **the suites that already exist**
|
|
under valgrind — `ctest -T memcheck` with the headless drivers forced, wrapped by
|
|
`scripts/memcheck.sh` so that a finding is an exit status rather than a line in a
|
|
log nobody reads. There are no memory-check test programs, and there should never
|
|
be any: `tests/benchutil.h` notices that it is running under valgrind and divides
|
|
every benchmark's iteration count by two thousand, which turns the perf suites
|
|
into the broadest path coverage in the tree at a cost valgrind can survive. The
|
|
whole run is about thirty seconds.
|
|
|
|
The two halves fit together on purpose. A benchmark is a program that walks one
|
|
path a hundred thousand times; a leak check wants every path walked once. Same
|
|
binaries, same registration, one flag apart.
|
|
|
|
Third-party findings are suppressed in `scripts/valgrind.supp`, and the run
|
|
forces `SDL_VIDEO_DRIVER=dummy` / `SDL_RENDER_DRIVER=software` /
|
|
`SDL_AUDIO_DRIVER=dummy` so the vendor GPU stack is never loaded — that removes
|
|
thousands of unfixable findings inside `amdgpu_dri.so` without suppressing
|
|
anything at all. Only *definite* losses and invalid accesses are counted; "still
|
|
reachable" is what SDL and FreeType keep for the process lifetime and says
|
|
nothing about this library.
|
|
|
|
### Defects the memory checker found
|
|
|
|
**All six are fixed.** They are kept here rather than deleted because the sizes
|
|
are measured and the reasoning is worth having next time somebody asks why a
|
|
loader ends in a `CLEANUP` block, or why a name field is staged through a zeroed
|
|
buffer. `cmake --build build --target memcheck` is clean, and the CI job that
|
|
runs it gates: the next one of these fails the build on the push that
|
|
introduces it.
|
|
|
|
Ordered by blast radius as they were found. Numbering continues the lists above.
|
|
Every size below is measured, not estimated.
|
|
|
|
34. **Every JSON loader leaks its parsed document.** There are four
|
|
`json_load_file` calls in `src/` and not one `json_decref` anywhere in the
|
|
library, so the whole parsed tree — objects, hashtables, strings — is
|
|
abandoned on both the success and failure paths:
|
|
|
|
| Loader | Site | Leaked per call |
|
|
|---|---|---:|
|
|
| `akgl_sprite_load_json` | `src/sprite.c:140` | ~1,500 bytes |
|
|
| `akgl_character_load_json` | `src/character.c:232` | ~2,150 bytes |
|
|
| `akgl_tilemap_load` | `src/tilemap.c:693` | ~9,000 bytes (2x2 fixture map) |
|
|
| `akgl_registry_load_properties` | `src/registry.c:134` | not exercised by any test |
|
|
|
|
Blast radius: every asset load, forever. The map figure is for the 2x2
|
|
fixture; a real map's JSON is the size of its layer data, so a 128x128 map
|
|
leaks on the order of a megabyte per load. A game that reloads a level on
|
|
death leaks a level's worth of JSON each time, and this is the one item in
|
|
this list that grows without bound.
|
|
|
|
**Fixed.** `json_decref` in the `CLEANUP` block of each, on the success
|
|
path as well as the failure one, with the handle nulled after so a second
|
|
pass cannot double-release. `akgl_registry_load_properties` needed its loop
|
|
moved inside the `ATTEMPT` block first: `props` is a borrowed reference into
|
|
the document and was read after the block ended, so every exit from that
|
|
loop leaked the document. The test is the memcheck run -- nothing in the
|
|
public API can observe a jansson refcount, and inventing a hook to prove it
|
|
would be testing the test.
|
|
|
|
35. **`akgl_get_property` reads up to 4 KiB past the end of the property
|
|
value.** `src/registry.c:181` copies a fixed `AKGL_MAX_STRING_LENGTH` bytes
|
|
out of whatever `SDL_GetStringProperty` returns, and what it returns is an
|
|
`SDL_strdup` of the value — four bytes for `"0.0"`. Valgrind reports an
|
|
invalid read on **every call**, twelve of them in `tests/physics.c` alone,
|
|
because `akgl_physics_init_arcade` reads six properties and
|
|
`akgl_render_init2d` reads two more.
|
|
|
|
This has been in `registry.h` as a `@note` about the copy being "a fixed
|
|
#AKGL_MAX_STRING_LENGTH bytes rather than the length" — filed as waste. It
|
|
is not waste, it is an out-of-bounds read: today it walks into the rest of
|
|
SDL's heap and returns garbage past the terminator, and on a value that
|
|
lands at the end of a page it is a segfault in a getter.
|
|
|
|
**Fixed.** The copy is bounded by the value's own length plus its
|
|
terminator, and a value too long for an akgl_String is now refused with
|
|
AKERR_OUTOFBOUNDS instead of silently truncated. The documented
|
|
AKERR_NULLPOINTER for "unset with no default" is unchanged -- it is raised
|
|
deliberately now rather than arriving from inside `aksl_memcpy`.
|
|
`tests/registry.c` fills the destination with a sentinel, reads a
|
|
three-byte property back, and asserts every byte past the terminator still
|
|
holds the sentinel; that assertion fails against the old code.
|
|
|
|
36. **The savegame name tables read past the end of every registry key, and
|
|
write what they find into the file.** `akgl_game_save_actorname_iterator`
|
|
(`src/game.c:219`) writes `AKGL_ACTOR_MAX_NAME_LENGTH` — 128 — bytes
|
|
starting at the key SDL handed it, and SDL allocated that key to fit the
|
|
name. Valgrind catches it on a 40-byte allocation. The three sibling
|
|
iterators do the same thing at `src/game.c:248`, `src/game.c:280` and
|
|
`src/game.c:308`, with 128, 512 and 128 byte fixed widths; only the actor
|
|
one is reached by the current tests, because the other registries are empty
|
|
in the save roundtrip.
|
|
|
|
Two consequences, and the second is the interesting one. The read can fault
|
|
if the key sits at the end of a page. And whatever it reads goes into the
|
|
save file, so a savegame contains up to 500 bytes of this process's heap per
|
|
registered object — anything that happened to be next to the key. That is a
|
|
file a player might send someone.
|
|
|
|
**Fixed.** All four iterators now write through `write_name_field`, which
|
|
stages the key into a zeroed fixed-width buffer, so the padding is
|
|
deterministic and nothing but the name leaves the process. A
|
|
negative-array-size typedef fails the build if any of the four widths ever
|
|
outgrows the staging buffer. Still open and unrelated to the overread:
|
|
**Defects -> Known and still open** item 7, the same tables disagreeing
|
|
about their widths between writer and reader.
|
|
|
|
37. **`akgl_controller_list_keyboards` leaks the array SDL gives it.**
|
|
`src/controller.c:188` calls `SDL_GetKeyboards`, which allocates, and never
|
|
calls `SDL_free` on the result. Four bytes per call in the test environment
|
|
— one keyboard id — but it is per call, and the function is shaped like
|
|
something a game calls when a device is hotplugged.
|
|
|
|
**Fixed.** One `SDL_free`, in a `CLEANUP` block so a failure inside the
|
|
loop cannot take the array with it. Still worth asking the same question of
|
|
every SDL enumeration in `src/controller.c`: `SDL_GetGamepads` has the same
|
|
contract, and the dummy driver reports no gamepads, so no test reaches it.
|
|
|
|
38. **A font, once loaded, is never freed and cannot be.**
|
|
`akgl_text_loadfont` (`src/text.c:20`) opens a `TTF_Font`, puts the pointer
|
|
in `AKGL_REGISTRY_FONT`, and that is the last anyone can do about it: the
|
|
header exposes no way to close a font, and `SDL_Quit` destroying the
|
|
property registry drops the last reference. 10,523 bytes per font — 736 of
|
|
them SDL_ttf's, the rest FreeType's.
|
|
|
|
This is bounded by how many fonts a game loads, so it is not the runaway
|
|
that item 34 is. It was a gap in the API rather than only a leak: a game
|
|
that switches fonts between scenes, or a tool like `charviewer` that reloads
|
|
one while the user picks a size, had no way to give the old one back.
|
|
|
|
**Fixed**, and this one is a new public symbol rather than a repair:
|
|
`akgl_text_unloadfont(char *name)` clears the registry entry and closes the
|
|
font. `akgl_text_loadfont` calls it when it replaces a live name, which
|
|
closes the second leak the header used to document as intended behaviour --
|
|
but only after the new font has opened, so a failed reload leaves the caller
|
|
with the font they already had. The version goes to 0.4.0 with it: an 0.3
|
|
consumer cannot be handed this library and told it is the same ABI.
|
|
|
|
Still open: nothing closes the registry's remaining fonts at shutdown. A
|
|
game that exits without unloading leaks them exactly once, which valgrind
|
|
reports against the process rather than against a loop, and which
|
|
`akgl_game_shutdown` would be the natural home for if it existed.
|
|
|
|
39. **Vendored `deps/semver`'s own unit test leaks 188 bytes** across 16 blocks,
|
|
from the `calloc`s in `test_strcut_first` and `test_strcut_second`
|
|
(`deps/semver/semver_unit.c:8` and `:21`). Not libakgl's code and not
|
|
libakgl's to fix; recorded so that nobody re-diagnoses it, and because
|
|
`semver_unit` is registered as one of our CTest tests and so shows up in our
|
|
memcheck run.
|
|
|
|
**Suppressed**, which is what this list said the answer would be if it ever
|
|
became noise, and gating the job made it noise. The two entries in
|
|
`scripts/valgrind.supp` name the functions rather than the file, so a
|
|
rewrite of those cases stops being suppressed and comes back as a finding.
|
|
They are the only entries there that hide a real leak in a program this
|
|
build runs; the alternative was a fork of a vendored dependency.
|
|
|
|
### Not defects, and why
|
|
|
|
- **`tests/util.c` fixtures were reading uninitialised stack floats.** Three
|
|
null-pointer tests declared `SDL_FRect` and `point` fixtures without
|
|
initializing them and then made one real call with them at the end, which is
|
|
sixteen "conditional jump depends on uninitialised value" findings for a test
|
|
that is not about coordinates. The fixtures are zeroed now. The library was
|
|
never at fault; a memory checker that reports noise gets ignored, which is the
|
|
only reason this was worth touching.
|
|
- **"Still reachable" at exit is not counted.** SDL's global state, the hint
|
|
table, the property registry and FreeType's library instance are one per
|
|
process and are reclaimed by `SDL_Quit` / `TTF_Quit`. Counting them would
|
|
bury the six findings above under a hundred that mean nothing.
|
|
- **The GPU driver's findings are avoided rather than suppressed.** Running the
|
|
suite against the real driver produces thousands of findings inside
|
|
`amdgpu_dri.so`; running it headless produces none, and headless is what the
|
|
suites are written for anyway.
|
|
|
|
## Build notes
|
|
|
|
The vendored SDL satellite libraries are built into per-project subdirectories
|
|
that are not on the loader's default search path, and `LD_LIBRARY_PATH` is
|
|
searched ahead of RPATH — so a developer who has previously run `rebuild.sh` had
|
|
their installed `libakgl.so` shadow the one under test, and a developer who had
|
|
not saw every test abort before `main()` with "cannot open shared object file".
|
|
`CMakeLists.txt` now sets `BUILD_RPATH` on the library, the utility, and every
|
|
test target, and prepends the build tree to `LD_LIBRARY_PATH` for the CTest run.
|
|
|
|
## API gaps blocking akbasic
|
|
|
|
`akbasic` (the C port of the BASIC interpreter, `source.starfort.tech/andrew/akbasic`) is
|
|
being built to link into `libakgl` as a scripting engine for game authors.
|
|
|
|
**All ten items are resolved.** Items 1 through 4 landed first and `akbasic` has since
|
|
consumed every one of them: its `libakgl`-backed text sink, its graphics backend, its sound
|
|
backend and its input backend are written and tested, and the BASIC 7.0 graphics verbs
|
|
(`GRAPHIC`, `COLOR`, `DRAW`, `BOX`, `CIRCLE`, `PAINT`, `SCALE`, `SSHAPE`, `GSHAPE`, `LOCATE`),
|
|
the sound verbs (`SOUND`, `ENVELOPE`, `VOL`, `PLAY`, `TEMPO`) and the console input verbs
|
|
(`GET`, `GETKEY`, `SCNCLR`) all work against them.
|
|
|
|
Doing that turned up **items 5 through 9**. Four were things `akbasic` had to work around to
|
|
build at all; each workaround is commented at its site over there with the words "filed
|
|
upstream". **Those workarounds can now be deleted** — including the five `add_subdirectory`
|
|
lines for the vendored SDL projects and the hand-assigned render vtable in
|
|
`tests/akgl_backends.c`.
|
|
|
|
Building the *standalone* SDL frontend on top of all that — a real window, a real event pump
|
|
and a line editor — turned up **item 10**, and re-confirmed items 6 and 7: both workarounds
|
|
had to be written a second time, in `src/frontend_akgl.c`, because a host that owns a window is
|
|
exactly the caller they inconvenience.
|
|
|
|
These were filed here rather than worked around in `akbasic` because growing `libakgl` to serve
|
|
a consumer is the wanted outcome. Each entry says what the BASIC verb needs, what the
|
|
`akgl_*` entry point should look like, and what would cover it.
|
|
|
|
Items 7, 9 and 10 add public symbols and item 9 adds three fields to `akgl_AudioVoice`, so the
|
|
project version goes to **0.3.0** and the soname with it — an 0.2 consumer cannot be handed
|
|
this library and told it is compatible.
|
|
|
|
1. **No way to measure rendered text.** `include/akgl/text.h` exposes `akgl_text_loadfont()`
|
|
and `akgl_text_rendertextat()`, and nothing that reports how large a string will be in a
|
|
given font. A terminal-style text surface cannot be built on that: a cursor needs the
|
|
advance width of one cell, and wrapping needs to know where a string crosses the right
|
|
margin. The reference interpreter got this from SDL2_ttf's `font.SizeUTF8("A")` and derived
|
|
its whole character grid from it.
|
|
|
|
Wants `akerr_ErrorContext AKERR_NOIGNORE *akgl_text_measure(TTF_Font *font, char *text, int *w, int *h);`
|
|
over `TTF_GetStringSize`, and probably a companion
|
|
`akgl_text_measure_wrapped(font, text, wraplength, w, h)` matching the `wraplength`
|
|
argument `akgl_text_rendertextat` already takes. Tests: a known string in a known font at a
|
|
known size, the empty string, and a wrapped string wide enough to force two lines.
|
|
|
|
This is the only one of the four that blocks work already designed and waiting. **`akbasic`
|
|
cannot render any output through `libakgl` until it lands.**
|
|
|
|
**Resolved.** `akgl_text_measure(font, text, w, h)` and
|
|
`akgl_text_measure_wrapped(font, text, wraplength, w, h)` are in
|
|
`include/akgl/text.h`, over `TTF_GetStringSize` and `TTF_GetStringSizeWrapped`.
|
|
Neither needs a renderer. A negative `wraplength` is refused with
|
|
`AKERR_OUTOFBOUNDS` rather than passed through, because SDL_ttf reads it as a
|
|
very large unsigned width and silently stops wrapping. `tests/text.c` covers
|
|
both against `tests/assets/akgl_test_mono.ttf`, a 10 KB monospaced ASCII
|
|
subset added for the purpose — being monospaced, it lets the suite assert
|
|
`width("AAAA") == 4 * width("A")` instead of hardcoding glyph metrics that
|
|
FreeType is free to round differently. Same change fixed item 39 below:
|
|
`akgl_text_loadfont` checked `name` twice and never checked `filepath`.
|
|
|
|
2. **No immediate-mode drawing.** `include/akgl/draw.h` declares exactly one function,
|
|
`akgl_draw_background(int w, int h)`, and `src/draw.c` is at 0% coverage. BASIC 7.0's
|
|
graphics verbs are all immediate-mode plotting against the current screen: `DRAW` (line and
|
|
point), `BOX`, `CIRCLE`, `PAINT` (flood fill), `LOCATE` (set the pixel cursor), `COLOR`,
|
|
and `SSHAPE`/`GSHAPE` (save and restore a rectangle of pixels).
|
|
|
|
Wants an `akgl_draw_*` family taking the renderer the host already initialized --
|
|
`akgl_draw_line`, `_rect`, `_filled_rect`, `_circle`, `_point`, `_flood_fill`,
|
|
`_copy_region` -- in the shape of the existing `akgl_render_2d_draw_texture`. SDL3's
|
|
`SDL_RenderLine`/`SDL_RenderRect`/`SDL_RenderFillRect` cover most of it; the circle and the
|
|
flood fill do not exist in SDL3 and need writing. Tests belong with the offscreen renderer
|
|
harness described under "Remaining work": render a known shape, read the target back, and
|
|
compare against a reference surface with the existing `akgl_compare_sdl_surfaces`.
|
|
|
|
**Resolved.** `include/akgl/draw.h` now declares `akgl_draw_point`, `_line`,
|
|
`_rect`, `_filled_rect`, `_circle`, `_flood_fill`, `_copy_region` and
|
|
`_paste_region`, all taking the `akgl_RenderBackend *` the host initialized,
|
|
in the shape of `akgl_render_2d_draw_texture`. Decisions worth knowing:
|
|
|
|
- **Color is an argument, not state.** There is no current-color global to
|
|
get out of step with the caller's own. Each call saves and restores the
|
|
renderer's draw color, so drawing a line does not change what the host's
|
|
next `SDL_RenderClear` paints. `tests/draw.c` asserts that.
|
|
- **The circle is a midpoint circle**, integer arithmetic with eight-way
|
|
symmetry, plotted eight points per step through `SDL_RenderPoints`.
|
|
- **The flood fill reads the target back, fills on the CPU, and blits only
|
|
the bounding box of what changed.** It keeps a fixed
|
|
`AKGL_DRAW_MAX_FLOOD_SPANS` (4096) stack of horizontal runs at file scope
|
|
rather than recursing per pixel; running out reports `AKERR_OUTOFBOUNDS`
|
|
and leaves the region partially filled, which is stated in the header. It
|
|
is therefore not reentrant — neither is anything else that draws to a
|
|
single `SDL_Renderer`.
|
|
- `_copy_region` allocates when `*dest` is `NULL` and otherwise copies into
|
|
the caller's surface, matching `akgl_get_json_string_value` and friends. A
|
|
region that would be clipped by the target edge is refused rather than
|
|
silently returning a smaller surface.
|
|
|
|
`tests/draw.c` draws into a 64x64 software renderer under the dummy video
|
|
driver and reads pixels back, so it did not need the offscreen harness. That
|
|
harness is still wanted for `src/renderer.c`, `src/text.c` and `src/assets.c`.
|
|
`akgl_draw_background` is untouched and still outside the error protocol
|
|
(item 35).
|
|
|
|
3. **No audio API at all.** `SDL3_mixer` is a vendored dependency and `registry.h` declares
|
|
`AKGL_REGISTRY_MUSIC`, but there is no `src/audio.c`, no `include/akgl/audio.h`, and no
|
|
`akgl_*` symbol that opens a mixer, loads a chunk, or plays a note. BASIC 7.0's sound verbs
|
|
are `SOUND` (a tone on a voice, with a duration), `PLAY` (a string of notes in a
|
|
Commodore-specific notation), `ENVELOPE` (ADSR per voice), `FILTER`, `VOL` and `TEMPO`.
|
|
|
|
`PLAY` and `ENVELOPE` want a synthesised voice rather than a sample, which SDL3_mixer does
|
|
not provide directly -- the honest first step is a small tone generator feeding
|
|
`SDL_AudioStream`, with `akgl_audio_init`, `akgl_audio_tone(voice, hz, ms)`,
|
|
`akgl_audio_envelope(voice, a, d, s, r)` and `akgl_audio_volume(level)`. This is the largest
|
|
of the four and the one most worth designing before writing. Tests can run under the dummy
|
|
audio driver and assert state transitions rather than sound.
|
|
|
|
**Resolved.** `include/akgl/audio.h` and `src/audio.c` add a three-voice tone
|
|
generator over `SDL_AudioStream`: `akgl_audio_init`, `_shutdown`, `_tone`,
|
|
`_stop`, `_waveform`, `_envelope`, `_volume`, `_voice_active` and `_mix`.
|
|
It is deliberately separate from the SDL3_mixer side of the library, which
|
|
plays audio *assets*; nothing in `audio.c` reads a file. Decisions worth
|
|
knowing:
|
|
|
|
- **The voice table works with no device open.** `akgl_audio_init` connects
|
|
it to one; without that a host can still pull samples itself through
|
|
`akgl_audio_mix`. That is not only for embedding — it is what makes the
|
|
suite deterministic. A device pulls samples on SDL's audio thread whenever
|
|
it likes, so a test that opened one and then asserted on voice state would
|
|
be racing the callback. `tests/audio.c` mixes by hand and opens a device
|
|
only in its last test.
|
|
- **Phase is derived from the frame counter, not accumulated.** A float
|
|
increment of `hz / 44100` is not exact, and adding it 44100 times a second
|
|
walks a held note off pitch.
|
|
- **A voice that was never configured is audible.** A zeroed voice has a
|
|
sustain of 0.0, which is silence with no error to explain it, so the table
|
|
defaults to a square wave held at full level.
|
|
- **Three voices summing past full scale are clamped, not scaled**, so one
|
|
voice plays at the level it was asked for rather than a third of it.
|
|
- Everything that touches the table takes the stream lock when a device is
|
|
open, since the callback reads it on another thread.
|
|
|
|
Still missing for a complete BASIC sound vocabulary: `FILTER` (SDL3 has no
|
|
filter primitive; this would need writing), `TEMPO` and the `PLAY` note-string
|
|
parser, both of which belong in the interpreter rather than here.
|
|
|
|
4. **No non-blocking keystroke read.** `include/akgl/controller.h` is built around SDL event
|
|
handlers the host pumps (`akgl_controller_handle_event` and friends), which suits a game
|
|
loop and does not suit `GET` and `GETKEY` -- those ask "is there a keystroke waiting, yes or
|
|
no" and must not require the interpreter to own the event loop. Goal 3 of `akbasic` forbids
|
|
it owning one.
|
|
|
|
Wants `akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_poll_key(int *keycode, bool *available);`
|
|
reading a small ring buffer that `akgl_controller_handle_event` already fills, so the host
|
|
keeps pumping events and the interpreter drains characters at its own pace. Tests: push
|
|
synthetic `SDL_EVENT_KEY_DOWN` events through the existing handler and drain them, plus the
|
|
empty-buffer and overflow cases.
|
|
|
|
**Resolved.** `akgl_controller_poll_key(int *keycode, bool *available)` and
|
|
`akgl_controller_flush_keys(void)` are in `include/akgl/controller.h`, over a
|
|
fixed `AKGL_CONTROLLER_KEY_BUFFER` (32) ring in `src/controller.c`.
|
|
`akgl_controller_handle_event` records every `SDL_EVENT_KEY_DOWN` *before* it
|
|
scans the control maps, so a key bound to an actor still reaches a polling
|
|
caller — the scan returns as soon as a binding claims the event, and doing it
|
|
afterwards would have lost exactly the keys a game also acts on. An empty
|
|
buffer is success with `available` false, not an error. A full buffer drops
|
|
the newest key rather than overwriting the oldest, matching the Commodore
|
|
keyboard buffer and keeping what was typed first. `tests/controller.c` covers
|
|
drain order, the release-is-not-a-keystroke case, a key shared with a control
|
|
map, flush, both NULL arguments, and overflow plus reuse afterwards.
|
|
|
|
5. **An embedded `libakgl` demands its dependencies be installed, while vendoring them.**
|
|
`CMakeLists.txt:17` gates the whole vendored-dependency block on
|
|
`CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR`, so `deps/SDL`, `deps/SDL_image`,
|
|
`deps/SDL_mixer`, `deps/SDL_ttf` and `deps/jansson` are added only when this repository is
|
|
the top-level project. Embedded with `add_subdirectory()`, the `else()` branch at `:51`
|
|
runs `find_package(SDL3 REQUIRED)` and friends instead, and configure fails on a machine
|
|
that has none of them installed — with the submodules sitting right there in `deps/`,
|
|
already checked out by the recursive clone the consumer just did.
|
|
|
|
`akbasic` works around it by adding those five subdirectories itself, immediately before
|
|
`add_subdirectory(deps/libakgl)`. That works only because every lookup in the `else()`
|
|
branch is guarded with `if(NOT TARGET ...)` — which is the same escape hatch
|
|
`akerror::akerror` and `akstdlib::akstdlib` already rely on, and it should not be the
|
|
documented answer. **Fix:** add the vendored dependencies on both paths, still guarded by
|
|
`if(NOT TARGET ...)` so a consumer that has already declared them wins. The suppression of
|
|
their CTest registration should move with them.
|
|
|
|
**Resolved.** The vendored block no longer sits behind
|
|
`CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR`. An
|
|
`akgl_add_vendored_dependency(<target> <dir>)` macro adds each of the seven submodules when
|
|
nothing has already declared that target *and* the submodule is actually checked out; the
|
|
`find_package` lookups for anything left over run afterwards, unchanged, so a checkout
|
|
without submodules still resolves against the system. A macro rather than a function
|
|
because `add_subdirectory()` inside a function runs in that function's variable scope.
|
|
|
|
The CTest suppression moved with them, and is lifted again before this project registers
|
|
its own suites, so an embedding consumer's `add_test()` still reaches CTest. Two smaller
|
|
consequences of the move: `find_package(PkgConfig)` now runs only when something has to
|
|
come from the system (a fully vendored build needs no pkg-config of its own, though
|
|
`deps/SDL_mixer` asks for it separately), and the build-tree RPATH block keys on whether
|
|
anything was vendored rather than on being the top-level project, so an embedded build's
|
|
tests can also find the satellite libraries.
|
|
|
|
Verified by configuring and building a scratch consumer that embeds this repository with
|
|
`CMAKE_DISABLE_FIND_PACKAGE_SDL3`, `_SDL3_image`, `_SDL3_mixer`, `_SDL3_ttf`, `_akerror`,
|
|
`_akstdlib` and `_jansson` all set, so any reliance on an installed copy would have failed
|
|
the configure. It configures, builds and links. **The five `add_subdirectory` lines in
|
|
`akbasic`'s `CMakeLists.txt` can be deleted.**
|
|
|
|
6. **`include/akgl/controller.h` does not compile on its own.** Lines 35, 36 and 41 declare
|
|
handler function pointers taking an `akgl_Actor *`, and the header includes only
|
|
`SDL3/SDL.h`, `akerror.h` and `types.h` — none of which declares that type. Any translation
|
|
unit that includes `akgl/controller.h` before `akgl/actor.h` fails with *"unknown type name
|
|
'akgl_Actor'"*. `src/controller.c` never notices because it includes `akgl/game.h` first.
|
|
|
|
This is the house rule in `AGENTS.md` — keep headers self-contained, include what you use.
|
|
**Fix:** `#include "actor.h"` in `controller.h`, or forward-declare
|
|
`struct akgl_Actor` if the include order makes that circular. A one-line test that includes
|
|
only `akgl/controller.h` would have caught it and would keep catching it.
|
|
|
|
**Resolved.** `controller.h` includes `<akgl/actor.h>`. Not a forward declaration:
|
|
`akgl_Actor` is a typedef of a named struct, and repeating a typedef is C11, not C99. The
|
|
include closes no cycle — `actor.h` reaches only `types.h` and `character.h`, neither of
|
|
which knows about the controller.
|
|
|
|
`tests/headers.c` is the test, and it is a whole suite for one `#include` on purpose: a
|
|
second `#include` after the first proves nothing about the second, because by then the
|
|
first has dragged its dependencies in. Covering another header means another file shaped
|
|
like that one. It also stopped being true that `tests/controller.c` has to include
|
|
`akgl/actor.h` first, and the comment there saying so is gone.
|
|
|
|
7. **There is no way to attach a 2D backend to a renderer the caller already has.**
|
|
`akgl_render_init2d()` (`src/renderer.c:17`) does two separable things: it creates a window
|
|
and an `SDL_Renderer` from the `game.screenwidth`/`game.screenheight` properties and writes
|
|
to the `camera` global, and it installs the six function pointers that make an
|
|
`akgl_RenderBackend` usable. A caller who already owns an `SDL_Renderer` wants only the
|
|
second half — and that caller is not hypothetical, it is precisely the embedding host the
|
|
API-gap section above exists to serve, since an embedded interpreter must not create the
|
|
window.
|
|
|
|
Today the only options are to call `akgl_game_init()` and let libakgl own the window, or to
|
|
assign the six pointers by hand, which is what `akbasic`'s `tests/akgl_backends.c` does.
|
|
**Fix:** split out `akgl_render_bind2d(akgl_RenderBackend *self)` that installs the vtable
|
|
and nothing else, and have `akgl_render_init2d()` call it after it has made its window.
|
|
`tests/renderer.c` could then cover the vtable half without a display at all.
|
|
|
|
**Resolved.** `akgl_render_bind2d(akgl_RenderBackend *self)` installs the six pointers and
|
|
returns; `akgl_render_init2d()` calls it once the window and the camera exist. It
|
|
deliberately does *not* touch `self->sdl_renderer`, which is the whole point — a host that
|
|
already owns one keeps it, and a host that does not gets a backend whose entry points all
|
|
report `AKERR_NULLPOINTER` instead of crashing.
|
|
|
|
`tests/renderer.c` is new and needs no display: it covers the binding (including that the
|
|
caller's `SDL_Renderer` survives it, and that binding a backend without one is legal),
|
|
frame start and end and both `draw_texture` paths against a software renderer under the
|
|
dummy video driver, the refusals a bound-but-unrendered backend gives, and the `draw_mesh`
|
|
"not implemented" stub. `src/renderer.c` goes from 10% line coverage to 56%; what is left
|
|
is `akgl_render_init2d` itself and `akgl_render_2d_draw_world`, both of which want the
|
|
offscreen harness and the world globals. **`akbasic`'s hand-assigned vtable in
|
|
`tests/akgl_backends.c` can be deleted.**
|
|
|
|
8. **`akgl_text_rendertextat()` dereferences an uninitialised backend vtable.**
|
|
`src/text.c` calls `renderer->draw_texture(renderer, ...)` with no NULL check on either
|
|
`renderer` or the function pointer, so a backend that has an `SDL_Renderer` but has not been
|
|
through `akgl_render_init2d()` segfaults on the first line of text. That is exactly the
|
|
state item 7 leaves a host in, and it is the same class of defect the draw commit at
|
|
42b60f7 added a test for — *"a backend that exists but was never given an SDL_Renderer,
|
|
which is the state a host is in between allocating one and initializing it; every draw
|
|
entry point has to report it rather than dereference it."* The text path still has it.
|
|
|
|
**Fix:** `FAIL_ZERO_RETURN` on `renderer`, on `renderer->sdl_renderer` and on
|
|
`renderer->draw_texture`, reporting `AKGL_ERR_SDL` or `AKERR_NULLPOINTER` as the draw
|
|
entry points do. Test alongside the existing `tests/draw.c` backend-without-a-renderer case.
|
|
|
|
**Resolved.** All three are checked, with `AKERR_NULLPOINTER` to match the draw entry
|
|
points, and they are checked *before* anything is rasterized rather than after: refusing
|
|
early costs nothing and leaks nothing, where refusing after the rasterize would hit the
|
|
leak the `@note` on this function already describes.
|
|
|
|
`tests/text.c` grew a software renderer under the dummy video driver — bound with the new
|
|
`akgl_render_bind2d`, which is what made this cheap — and covers all three refusals plus
|
|
the successful draw, wrapped and unwrapped. `src/text.c` goes from 58% to **100%** line
|
|
coverage, and the three `akgl_text_rendertextat` mutants the mutation run left surviving
|
|
are dead. The third case is the one that matters: with a *live* `SDL_Renderer` behind a
|
|
backend that was never bound, the old code got all the way to a NULL function pointer.
|
|
Checking it against a made-up renderer pointer, as the first draft of the test did, is
|
|
worth nothing — SDL refuses the bogus handle and the call fails for the wrong reason, which
|
|
a deleted-check mutant survives.
|
|
|
|
Found while writing that test and filed below as item 27: SDL_ttf refuses the empty string
|
|
on both rasterizing paths, so drawing an empty line is an error while measuring one is not.
|
|
|
|
9. **`SOUND`'s frequency sweep has no `akgl_audio_*` equivalent.** BASIC 7.0's
|
|
`SOUND voice, freq, dur, dir, min, step` ramps the pitch from `freq` toward `min` in `step`
|
|
increments per tick, in the direction `dir` selects — a siren, a laser, the whole reason
|
|
`SOUND` has six arguments. `akgl_audio_tone(voice, hz, ms)` holds one pitch for one
|
|
duration, and nothing changes pitch over the life of a note.
|
|
|
|
`akbasic` refuses those three arguments rather than faking them, and the reasoning is worth
|
|
recording because it is what makes this a `libakgl` gap rather than an interpreter one: the
|
|
only way to fake a sweep from the interpreter is to re-issue tones from its step loop, which
|
|
ties audible pitch to how often the host happens to call it — a tune that changes key with
|
|
the frame rate. It has to be advanced on the mixer's own frame counter, which is where the
|
|
phase is already derived from and which only this library can see.
|
|
|
|
**Fix:** `akgl_audio_sweep(int voice, float32_t from_hz, float32_t to_hz, float32_t step_hz, uint32_t ms)`,
|
|
advanced in `akgl_audio_mix()` beside the envelope. Tests would drive `akgl_audio_mix` by
|
|
hand and assert the frame at which the pitch has moved, exactly as `tests/audio.c` already
|
|
does for the envelope stages.
|
|
|
|
**Resolved.** `akgl_audio_sweep(voice, from_hz, to_hz, step_hz, ms)` is in
|
|
`include/akgl/audio.h`, with the step taken in `akgl_audio_mix()` off the mixer's own frame
|
|
counter. Decisions worth knowing:
|
|
|
|
- **One step every 1/60 second** (`AKGL_AUDIO_SWEEP_TICK_HZ`), because that is the rate the
|
|
machine this vocabulary comes from advanced its sweep at, and its tunes are written for
|
|
it. It divides 44100 exactly, so a step boundary always lands on a whole frame — which is
|
|
what lets the suite assert the exact frame the pitch moves on rather than a tolerance.
|
|
- **Direction comes from the two frequencies, not from the sign of the step.** `step_hz`
|
|
stays positive; `to_hz` below `from_hz` sweeps down. A negative step is refused rather
|
|
than silently meaning something.
|
|
- **It arrives and holds.** The last step is clamped to `to_hz` rather than overshooting,
|
|
and equal frequencies are a legal held tone rather than an error, so a caller computing
|
|
its own limits does not have to special-case them.
|
|
- **`akgl_audio_tone` is now the same start path with a step of 0**, which is also what
|
|
makes a voice reused for a plain tone stop sweeping. That is one function, `start_note`,
|
|
rather than two copies of the same six assignments.
|
|
- **A swept voice accumulates its phase** instead of deriving it from the frame counter.
|
|
Deriving assumes a constant frequency; under a sweep it jumps the waveform at every step,
|
|
which is an audible click. The drift the derived form exists to avoid is not something a
|
|
note that is changing pitch anyway can be said to suffer from.
|
|
|
|
`tests/audio.c` drives the mixer by hand: one tick's frames do not move the pitch, the next
|
|
frame does, both directions clamp at their target, a gate shorter than the sweep cuts it off
|
|
part way, and a plain tone on a swept voice stays put. `src/audio.c` holds at 92%.
|
|
|
|
Still missing for a complete `SOUND`: the oscillating third direction (C128 `dir` 2), which
|
|
is a re-issue on arrival rather than a third kind of sweep, and `FILTER`, `TEMPO` and the
|
|
`PLAY` note-string parser as before.
|
|
|
|
10. **The keystroke ring carries a keycode and nothing else, so Shift is invisible.**
|
|
`akgl_controller_handle_event()` (`src/controller.c:104-105`) pushes `event->key.key` into
|
|
the ring and drops the rest of the event, and `akgl_controller_poll_key(int *keycode, bool
|
|
*available)` hands back only that. Item 4 asked for a non-blocking keystroke read and got
|
|
one; what it did not ask for, and what a *line editor* turns out to need, is which
|
|
character the keystroke actually produced.
|
|
|
|
`akbasic` has now built one — an `INPUT` and a REPL prompt drawn in the window, in
|
|
`src/sink_akgl.c` — and the consequence is concrete: no shifted character is reachable, so
|
|
`"`, `!`, `(`, `)`, `:` and `;` cannot be typed at all, and lower case cannot be typed
|
|
either. A BASIC line editor that cannot type a double quote cannot enter a string literal.
|
|
The interpreter folds letters to upper case, which is what a C128 does anyway and is the
|
|
right resolution for letters; it is not a resolution for punctuation, and there is nothing
|
|
the caller can do about it because the modifier state was discarded two layers down.
|
|
|
|
Note this is not solvable by the caller polling `SDL_GetModState()` at read time: by then
|
|
the key is long released, and the whole point of a ring is that reads are decoupled from
|
|
events.
|
|
|
|
**Fix:** widen the ring entry to carry the modifier state and the composed text SDL already
|
|
computes. Either
|
|
|
|
```c
|
|
typedef struct akgl_Keystroke {
|
|
SDL_Keycode key;
|
|
SDL_Keymod mod;
|
|
char text[8]; /* UTF-8, from SDL_EVENT_TEXT_INPUT; "" for a non-printing key */
|
|
} akgl_Keystroke;
|
|
|
|
akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_poll_keystroke(akgl_Keystroke *dest, bool *available);
|
|
```
|
|
|
|
alongside the existing `akgl_controller_poll_key()`, which stays as it is so nothing
|
|
breaks — a game asking "was the up arrow pressed" wants exactly what it already gets.
|
|
Filling `text` means handling `SDL_EVENT_TEXT_INPUT` as well as `SDL_EVENT_KEY_DOWN`, which
|
|
is the only correct way to get a character out of SDL anyway: it is what makes a keyboard
|
|
layout, a compose key and a dead key work, none of which a keycode can express.
|
|
|
|
Tests would push a shifted key-down plus its text-input event through
|
|
`akgl_controller_handle_event()` and assert both the keycode and the composed character
|
|
come back, mirroring what `tests/controller.c` already does for the plain ring.
|
|
|
|
**Resolved.** `akgl_Keystroke` (keycode, `SDL_Keymod`, and eight bytes of UTF-8) and
|
|
`akgl_controller_poll_keystroke(akgl_Keystroke *dest, bool *available)` are in
|
|
`include/akgl/controller.h`; the ring now holds those instead of bare keycodes.
|
|
`akgl_controller_poll_key()` is untouched from its caller's side. Decisions worth knowing:
|
|
|
|
- **The text is attached to the press that is still waiting for it**, tracked by a flag
|
|
rather than by "whatever entry is newest". Without the flag, a press dropped by a full
|
|
buffer hands its text to some older key that happens to be sitting at the end of the
|
|
ring — which is wrong in exactly the case where things are already going badly.
|
|
- **Text with no press behind it is buffered on its own**, with a keycode of 0. An input
|
|
method commit and a character finished by a dead key have no key press of their own, and
|
|
a line editor wants the character regardless. `akgl_controller_poll_key()` discards those
|
|
on the way past rather than reporting a keystroke with no key.
|
|
- **Oversized text is cut on a code point boundary.** An IME can commit several characters
|
|
at once and an entry holds one; truncating on a byte boundary would leave a partial UTF-8
|
|
sequence that nothing downstream can render.
|
|
- **SDL only sends `SDL_EVENT_TEXT_INPUT` while text input is started**, so a host that
|
|
wants `text` populated calls `SDL_StartTextInput()` on its window. Without it `key` and
|
|
`mod` still arrive. That is documented on the function rather than left to be discovered.
|
|
|
|
`tests/controller.c` covers the shifted-key case end to end (the `"` that started this),
|
|
a non-printing key composing to nothing, a text-only entry through both pollers, text that
|
|
outlived its own press, the code-point-boundary truncation, and both NULL arguments.
|
|
`src/controller.c` holds at 91%.
|
|
|
|
## Carried over
|
|
|
|
1. **Make character-to-sprite state bindings release their references symmetrically.**
|
|
`akgl_character_sprite_add()` increments the sprite refcount for every state-map
|
|
binding, but no corresponding removal API exists. Replacing a state binding
|
|
also leaves the previous sprite's refcount incremented. When
|
|
`akgl_heap_release_character()` drops the character refcount to zero, it clears
|
|
the character registry entry and zeroes the structure without enumerating
|
|
`state_sprites`, decrementing the bound sprites, or destroying the SDL property
|
|
map. Implement binding removal/replacement so each removed binding releases
|
|
exactly one sprite reference. On final character release, enumerate every
|
|
remaining binding (the existing `akgl_character_state_sprites_iterate()` release
|
|
path may be reusable), release each reference, destroy `state_sprites`, and then
|
|
clear the character. Add tests for removal, replacement, duplicate sprite
|
|
bindings across multiple states, and final character release.
|
|
|
|
`akgl_character_state_sprites_iterate` is still uncovered and is the natural
|
|
place to start; `tests/actor.c` now covers `akgl_character_sprite_get` and the
|
|
composite-state binding behavior it depends on.
|