diff --git a/TODO.md b/TODO.md index d9088eb..f6632da 100644 --- a/TODO.md +++ b/TODO.md @@ -1,5 +1,369 @@ # 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__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 + ``. 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 `. + +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 ``; `json_helpers.h` uses `json_t` without ``; + `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 `. 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. + +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: @@ -195,6 +559,33 @@ Each was found by a test written to assert correct behavior. 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. ## Build notes