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

Namespace every exported symbol, and bump to 0.5.0

Browse Source 
Closes internal-consistency items 1 through 6, 12 and 13. Every include guard
is _AKGL_<FILE>_H_, every in-project header include is angled, and every
exported function, type and global carries the akgl_ prefix. This is an ABI
break; the soname goes to libakgl.so.0.5. TODO.md carries the full rename
table.

The renames were driven by renaming each declaration and letting the compiler
find the uses, not by pattern substitution: renderer, physics and camera are
also parameter and struct-member names, and a sed would have rewritten
map->physics and every akgl_RenderBackend *renderer parameter without a word.

Item 4 turned out not to be cosmetic. The library exported a global called
renderer and tests/character.c defined an SDL_Renderer *renderer of its own;
the executable's definition preempted the library's, akgl_sprite_load_json
read a SDL_Renderer * through an akgl_RenderBackend *, and every texture load
in that suite failed. The suite reported success anyway, because libakerror's
unhandled-error handler ends in exit(errctx->status), exit keeps only the low
byte, and AKGL_ERR_SDL is exactly 256. So character had been green while
running one of its four tests, and every suite in the tree was unable to fail
on the most common status in a library built on SDL.

Both are fixed. tests/testutil.h gains TEST_TRAP_UNHANDLED_ERRORS(), which
collapses any status a byte cannot carry onto 1, and every suite installs it.
character binds a real backend with akgl_render_2d_bind. Its fourth test then
runs for the first time and fails on a defect it has asserted all along, so
akgl_heap_release_character now walks state_sprites with
AKGL_ITERATOR_OP_RELEASE and destroys the property set before zeroing the slot
-- TODO.md Defects item 21 and half of Carried over item 1.

AKGL_TIME_ONESEC_MS said "one second in milliseconds" and held 1000000, so
akgl_game_state_lock waited roughly sixteen minutes rather than one second. It
is AKGL_TIME_ONEMS_NS now, the budget is its own named constant, and
tests/game.c holds the mutex from a second thread to assert the wait -- the
contended path had no coverage at all.

Headers are self-contained and it is enforced: AKGL_PUBLIC_HEADERS drives both
install() and a generated translation unit per header, so a header that ships
is a header that is checked. Writing that found registry.h, which used
SDL_PropertiesID in eight declarations and included no SDL header.

23/23 suites pass, memcheck is clean, reindent --check is clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Andrew Kesterson
2026-07-31 23:32:21 -04:00
parent 93e8e4afa4
commit 9924d74dcc
60 changed files with 1199 additions and 860 deletions
Download Patch File Download Diff File Expand all files Collapse all files

224
TODO.md
View File

@@ -9,67 +9,62 @@ 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.
**Items 1 through 6 are resolved in 0.5.0**, which is an ABI break and carries
the soname to `libakgl.so.0.5`. What changed, and what a consumer has to rename:
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>`.
| Was | Is |
|---|---|
| `_ASSETS_H_`, `_CONTROLLER_H_`, `_DRAW_H_`, `_ERROR_H_`, `_JSON_HELPERS_H_`, `_PHYSICS_H_`, `_REGISTRY_H_`, `_RENDERER_H_`, `_TEXT_H_`, `_TILEMAP_H_`, `_UTIL_H_`, `_STRING_H_` | `_AKGL_<FILE>_H_` |
| `akgl_Actor_cmhf_*` (8 functions) | `akgl_actor_cmhf_*` |
| `akgl_game_updateFPS` | `akgl_game_update_fps` |
| `akgl_render_init2d`, `akgl_render_bind2d` | `akgl_render_2d_init`, `akgl_render_2d_bind` |
| `akgl_sprite_sheet_coords_for_frame` | `akgl_spritesheet_coords_for_frame` |
| `point`, `RectanglePoints` | `akgl_Point`, `akgl_RectanglePoints` |
| `window`, `bgm`, `game`, `gamemap`, `renderer`, `physics`, `camera` | the same, `akgl_`-prefixed |
| `_akgl_renderer`, `_akgl_physics`, `_akgl_camera`, `_akgl_gamemap` | `akgl_default_renderer`, `akgl_default_physics`, `akgl_default_camera`, `akgl_default_gamemap` |
| `HEAP_ACTOR`, `HEAP_SPRITE`, `HEAP_SPRITESHEET`, `HEAP_CHARACTER`, `HEAP_STRING` | `akgl_heap_actors`, `akgl_heap_sprites`, `akgl_heap_spritesheets`, `akgl_heap_characters`, `akgl_heap_strings` |
| `GAME_ControlMaps` | `akgl_controlmaps` |
| `AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH` | `AKGL_CHARACTER_MAX_NAME_LENGTH` |
| `AKGL_TIME_ONESEC_MS` | `AKGL_TIME_ONEMS_NS` |
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`.
`include/akgl/staticstring.h` also stopped guarding with `_STRING_H_` — a name
several libc implementations use for their own `<string.h>` — and its
`#include "string.h"` is now `#include <string.h>`.
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`.
`akgl_render_bind2d` was not on the original list. It is the same defect as
`akgl_render_init2d`, `2d` trailing where the six functions it installs carry it
in the middle, and renaming one without the other would have been worse than
renaming neither.
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.
The renames were done by renaming each declaration and letting the compiler find
every use, rather than by pattern substitution. That distinction matters for
`renderer`, `physics` and `camera`, which are also parameter and struct-member
names: a `sed` would have rewritten `map->physics` and every
`akgl_RenderBackend *renderer` parameter, and nothing would have complained.
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`.
**Item 6 had a live bug behind the name**, now fixed. `akgl_game_state_lock`
counted its retry loop against a constant named "one second in milliseconds"
that held `1000000`, so it retried 10,000 times at 100 ms and blocked for
roughly sixteen minutes before reporting failure. The budget is now
`AKGL_GAME_STATE_LOCK_BUDGET_MS` (1000), with the cadence named separately as
`AKGL_GAME_STATE_LOCK_RETRY_MS`. `tests/game.c` carries
`test_game_state_lock_budget`, which holds the mutex from a second thread and
asserts the call gives up between half a second and five. The old code fails it
by timing the suite out; a build that gave up without waiting at all fails the
lower bound. Nothing exercised the contended path before — the existing lock
test only ever took an uncontended mutex, which never reaches the retry loop.
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.
**Item 4 was not cosmetic, and the proof was sitting in the test suite.**
`renderer` was exported from the shared library, and `tests/character.c` defined
an `SDL_Renderer *renderer` of its own. Both had external linkage and the same
spelling, so the executable's definition preempted the library's:
`akgl_sprite_load_json` read a `SDL_Renderer *` through an
`akgl_RenderBackend *`, every texture load in that suite failed with
`Parameter 'renderer' is invalid`, and the suite still reported success — see
"Test suites that could not fail" below. It now binds a real backend with
`akgl_render_2d_bind`, the way `tests/sprite.c` and `tests/text.c` do. A
consuming game with a variable called `renderer` would have hit the same thing
with no test to notice.
### 2. Header/implementation surface drift
@@ -114,7 +109,7 @@ overlap the existing **Defects** list are cross-referenced rather than repeated.
|---|---|
| `character.h:41` `basechar` | `character.c:21` `obj` |
| `character.h:69` `props` | `character.c:75` `registry` |
| `heap.h:121` `ptr` | `heap.c:143` `basechar` |
| ~~`heap.h:121` `ptr`~~ | ~~`heap.c:143` `basechar`~~ — fixed in 0.5.0 |
| `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` |
@@ -131,19 +126,27 @@ overlap the existing **Defects** list are cross-referenced rather than repeated.
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.
12. **Headers rely on their includers for types.** `iterator.h` used `uint32_t`
without `<stdint.h>`; `json_helpers.h` used `json_t` without `<jansson.h>`;
`util.h` used `SDL_FRect` and `bool` without any SDL include. Each compiled
only because of `.c`-file include ordering.
**Resolved, and enforced rather than merely fixed.** `AKGL_PUBLIC_HEADERS`
in `CMakeLists.txt` is now the single list behind both `install()` and a
generated translation unit per header — each including exactly that header
and nothing before it — linked into the `headers` suite. A header that ships
is a header that is checked, and adding one to that list is all a new header
needs.
Writing the check found a case this item had missed: `registry.h` uses
`SDL_PropertiesID` in eight declarations and included no SDL header at all.
That is the argument for generating the check off the install list instead
of hand-listing the headers thought to be at risk.
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.
directory.** **Resolved.** Every in-project include in a public header uses
`#include <akgl/sibling.h>`. `tests/*.c` still use `#include "testutil.h"`,
correctly: that one is a test-local header rather than an installed one.
14. **Empty parameter lists.** `akgl_heap_init()`, `akgl_heap_init_actor()`,
`akgl_registry_init*()`, `akgl_game_init()`, `akgl_game_init_screen()`, and
@@ -367,6 +370,42 @@ overlap the existing **Defects** list are cross-referenced rather than repeated.
41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and the
`akgl_path_relative*` family use `dst`; everything else uses `dest`.
## Test suites that could not fail
Found while renaming the exported globals, and the reason item 4 above is filed
as a real defect rather than a style complaint. Both are fixed.
**Every suite reported success on any status whose low byte is zero.**
libakerror's default unhandled-error handler ends in `exit(errctx->status)`.
`exit` keeps only the low byte of what it is given, and libakgl's status band
starts at `AKERR_FIRST_CONSUMER_STATUS`, which is 256. `AKGL_ERR_SDL` is
therefore exactly 256, `exit(256)` is a wait status of 0, and CTest recorded a
pass. The most common failure status in a library built on SDL was the one
status that could not fail a test.
**`tests/character.c` was green while running one of its four tests.** It
aborted in `test_character_sprite_mgmt` with
`Failed loading asset .../spritesheet.png : Parameter 'renderer' is invalid` —
the symbol collision described in item 4 — and exited 0 because that status was
`AKGL_ERR_SDL`. Two independent defects had to line up, and they did, for long
enough that `TODO.md` recorded the suite as passing and wondered which change
had fixed it. Nothing had; it had stopped running.
**Fixed** in `tests/testutil.h`: `TEST_TRAP_UNHANDLED_ERRORS()` installs a
handler that collapses any status a byte cannot carry onto 1, and every suite's
`main()` calls it immediately after `akgl_error_init()`. `tests/error.c` calls
it after its first test instead, because that suite has no `akgl_error_init()`
in `main()` and libakerror's lazy `akerr_init()` would overwrite the handler.
Once installed, no other suite changed colour, so this was not masking anything
beyond `character` — but it could have been at any time, and nothing would have
said so.
Worth knowing: the fix belongs here rather than in libakerror only because
`exit(status)` is a reasonable thing for a library to do when statuses fit in a
byte. libakerror's own band does. Consumers' bands start at 256 by construction,
so any consumer's test suites have this problem. That is worth raising upstream.
## Coverage status
Generated with:
@@ -833,16 +872,25 @@ without coming here first. Ordered by blast radius.
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.
`src/heap.c:150` zeroed the character without walking `state_sprites`, so
every sprite reference the character took in `akgl_character_sprite_add` was
lost and the `SDL_PropertiesID` holding the map was never destroyed. Loading
and releasing characters in a loop — level to level — exhausted the sprite
pool and leaked an SDL property set each time.
Fix: enumerate `state_sprites` with that iterator, then
`SDL_DestroyProperties`, before the `memset`. Touches `src/heap.c:145-151`.
**Fixed in 0.5.0.** At a refcount of zero it enumerates `state_sprites` with
`akgl_character_state_sprites_iterate` and `AKGL_ITERATOR_OP_RELEASE`, then
`SDL_DestroyProperties`, then zeroes the slot. The iterator existed for
exactly this walk and simply was never called from here.
The test was already written. `tests/character.c` has asserted this contract
all along — "character did not reduce reference count of its child sprites
when released" — and had never once run, for the two reasons under "Test
suites that could not fail". It runs now, and it fails against the old code.
Still open and separate: **item 20**, `akgl_character_sprite_add` leaking the
displaced sprite when a state is *remapped*. Releasing at character teardown
does not cover a binding replaced while the character is alive.
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
@@ -1016,11 +1064,9 @@ Ordered by blast radius. Numbering continues the **Defects** list above.
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.
**Fixed in 0.5.0**; see item 21, which is the same defect. The
character-load benchmark in `tests/perf_render.c` was written around it and
no longer has to be.
### Targets
@@ -1745,9 +1791,19 @@ this library and told it is compatible.
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.
**The final-release half is done in 0.5.0** — see **Defects** item 21.
`akgl_heap_release_character` now enumerates every remaining binding, releases
each reference, destroys `state_sprites`, and then clears the character, and
`akgl_character_state_sprites_iterate` is covered by that path.
**Still open: removal and replacement.** There is no API to unbind one state,
and `akgl_character_sprite_add` still overwrites an existing entry without
releasing the sprite it displaces (**Defects** item 20). A character that
rebinds a state while alive still leaks one reference per rebind; teardown
only gives back whatever the map happens to hold at the end. Wants
`akgl_character_sprite_del`, a release of the displaced entry inside
`akgl_character_sprite_add`, and tests for removal, replacement, and
duplicate sprite bindings across multiple states.
2. **An actor cannot be scaled per axis.** `akgl_Actor::scale` is one `float32_t` applied
to both `dest.w` and `dest.h`, so there is no way to express "twice as wide, the same