From 772f960865a492edc9301c15f89ae81ccf00dd69 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Thu, 30 Jul 2026 18:17:58 -0400 Subject: [PATCH] Record internal consistency findings and the controller DB defect Add an "Internal consistency" section to TODO.md: 41 numbered findings from a sweep of src/ and include/, each cited by file:line and grouped by blast radius. These are convention problems, not a re-audit of behavior, but several have live functional consequences: - AKGL_TIME_ONESEC_MS is misnamed. Its value is nanoseconds-per- millisecond, but akgl_game_state_lock uses it as a one-second budget against a 100ms delay loop, so the lock blocks ~16 minutes. - SUCCEED_RETURN inside an ATTEMPT block at tilemap.c:52 bypasses the CLEANUP two lines below, leaking two heap strings on every successful tilemap property lookup, against a 256-entry pool. - AKGL_ACTOR_STATE_STRING_NAMES is declared [33] and defined [32], and names bits 11 and 12 as UNDEFINED where actor.h defines MOVING_IN and MOVING_OUT, so no character JSON can bind a sprite to those states. - 19 non-static functions are defined in src/ but declared in no header, and akgl_game_init_screen is declared but never defined. - AKGL_COLLIDE_RECTANGLES has unbalanced parentheses and cannot compile. - text.c:19 checks `name` twice and never validates `filepath`. Item 30 is marked resolved by the reindent in the preceding commit. Also add Defects #12: a failed controller-DB fetch silently destroys the tracked fallback. include/akgl/SDL_GameControllerDB.h is committed on purpose so the library still builds if upstream disappears, but mkcontrollermappings.sh has no `set -e` and never checks curl's exit status -- on failure it overwrites the good header with AKGL_SDL_GAMECONTROLLER_DB_LEN 0 and an empty initializer, and exits 0. Verified against an unresolvable host. CMakeLists.txt:89-93 compounds this: the custom command's OUTPUT is relative, so CMake resolves it against the binary dir while the script writes to the source dir, leaving the command permanently out of date and re-running on every build. Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 391 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 391 insertions(+) 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