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

Give every exported function a declaration, and check that it stays that way

Browse Source 
Closes internal-consistency items 7 through 15. Nineteen non-static functions
were in the ABI with no declaration anywhere, so no consumer could call them
and any consumer could collide with them.

The four gamepad_handle_* functions are the ones that mattered: controller.h
declared akgl_controller_handle_button_down and three siblings that did not
exist, so anything compiled against the header alone failed to link. The
definitions carry the declared names now, which also closes Defects -> Known
and still open item 10, and their documentation moved to the header.

The rest are either declared under a "part of the internal API" block --
akgl_game_save_actors and akgl_game_load_versioncmp, which tests/game.c had to
declare for itself, plus six tilemap loader helpers the untested-loader work
wants to reach -- or static, which is what the four save iterators and
load_objectnamemap should always have been. akgl_path_relative_from is deleted:
declared nowhere, called from nowhere, never wrote its output, and leaked a
pooled string on every call, so it closes Known and still open item 4 and item
40 by ceasing to exist.

scripts/check_api_surface.sh keeps it closed. It reads the built library's
dynamic symbol table and every public header with comments stripped, and fails
on an exported akgl_* symbol that is declared nowhere. Stripping comments is
the whole point -- four of these were mentioned in controller.h prose, which is
how they went unnoticed.

The pool-size ceilings are defined once, in heap.h, so the #ifndef override
hook fires for the first time; actor.h, sprite.h and character.h were defining
the same four unconditionally from headers heap.h includes above its own guard.
tests/header_pool_override.c fails the compile if that regresses.

Also here: (void) rather than () on the twelve no-argument entry points,
AKERR_NOIGNORE only on declarations, static helpers with the akgl_ prefix
dropped, and the six parameter-name mismatches. akgl_get_json_with_default had
its two contexts swapped rather than merely misspelled -- the incoming one was
`err` and its own was `e`, which is the name reserved for an incoming one.

24/24 pass, reindent --check clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Andrew Kesterson <andrew@aklabs.net>
This commit is contained in:
Tachikoma
2026-07-31 23:44:38 -04:00
parent e8cdca6fde
commit 76d39f09d4
29 changed files with 676 additions and 620 deletions
Download Patch File Download Diff File Expand all files Collapse all files

197
TODO.md
View File

@@ -68,100 +68,110 @@ with no test to notice.
### 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:
**Items 7, 8, 9, 11, 12, 13, 14 and 15 are resolved in 0.5.0.** Item 10 is
resolved for every pair it listed.
`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`).
7. **Nineteen non-static functions were defined in `src/` but declared in no
header.** Each is now declared or `static`:
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.
- `akgl_controller_handle_button_down`, `_button_up`, `_added`, `_removed`
were defined as `gamepad_handle_*` while `controller.h` declared the
`akgl_controller_handle_*` names it never defined. The definitions now
carry the declared names, which closes this and **Defects → Known and
still open item 10** in one change, and their documentation moved from the
definitions to the header where the convention puts it. `tests/controller.c`
no longer needs its local re-declarations.
- `akgl_game_save_actors` and `akgl_game_load_versioncmp` are declared in
`game.h` under a new "part of the internal API" block; `tests/game.c`
reaches them through the header now instead of declaring them itself.
- The four save-table iterators and `akgl_game_load_objectnamemap` are
`static` and have dropped the prefix -- `save_actorname_iterator`,
`load_objectnamemap` and so on. They are SDL enumeration callbacks and a
file-local reader; nothing outside `game.c` has any business calling them.
- `akgl_get_json_properties_number`, `_float`, `_double`,
`akgl_tilemap_load_layer_image`, `akgl_tilemap_load_layer_object_actor` and
`akgl_tilemap_load_physics` are declared in `tilemap.h`'s internal-API
block, again with their documentation moved to the header. That is where
the "does not need a renderer" work under **Remaining work** wanted them.
- `akgl_path_relative_root` is `static path_relative_root`.
`akgl_path_relative_from` is **deleted**; see below.
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.
**This is enforced now.** `scripts/check_api_surface.sh` reads the built
library's dynamic symbol table, strips comments out of every public header,
and fails when an exported `akgl_*` symbol is declared nowhere. It runs as
the `api_surface` CTest test. Stripping comments is the point: four of these
symbols were *mentioned* in `controller.h` prose, which is not the same as
being declared there and is exactly how they went unnoticed.
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.
8. **`akgl_game_init_screen` was declared and never defined.** The declaration
is gone. Screen setup is `akgl_render_2d_init`.
10. **Parameter names disagree between declaration and definition.** Doxygen
documents the header spelling, so the generated docs describe names the
implementation does not use:
9. **Static helpers used three naming styles.** They drop the `akgl_` prefix
now, which is what it is for: `character_load_json_inner`,
`character_load_json_state_int_from_strings`, `sprite_load_json_spritesheet`,
alongside the `actor_visible`, `write_exact` and `write_name_field` that
already did.
| 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`~~ — 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` |
10. **Parameter names disagreed between declaration and definition.** All six
pairs agree now. `akgl_character_initialize` takes `obj`,
`akgl_character_state_sprites_iterate` takes `props`, `akgl_heap_release_character`
takes `ptr`, `akgl_set_property` takes `value`, and the two `akgl_tilemap_draw*`
functions take `map` -- the header called them `dest` and documented them as
"Output destination populated by the function", which they are not.
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".
`akgl_get_json_with_default` was the interesting one: it took the incoming
context as `err` and named its *own* context `e`, which is the name the
convention reserves for an incoming one. It is `e` and `errctx` now, and
renaming it was what surfaced that the two had been swapped rather than
merely misspelled.
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.
11. **Object-pool size macros were defined twice and the override hook was
dead.** `AKGL_MAX_HEAP_ACTOR`, `_SPRITE`, `_SPRITESHEET` and `_CHARACTER` are
defined once, in `heap.h`, inside the `#ifndef` guards that were always
supposed to make them overridable. `actor.h`, `sprite.h` and `character.h`
no longer define them.
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.
`tests/header_pool_override.c` is the regression test: it defines its own
ceilings, includes `heap.h`, and `#error`s if the guard did not fire or if
`AKGL_MAX_HEAP_SPRITE` stopped deriving from `AKGL_MAX_HEAP_ACTOR`. The
assertion is the compile -- it deliberately disagrees with the built
library's ceilings and never touches the pools, which is fine because
overriding a ceiling means the library and everything linking it have to be
rebuilt together anyway.
12. **Headers relied 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.
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.
Writing the check found a case this item had missed: `registry.h` uses
Writing that 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.
That is the argument for generating the check off the install list rather
than hand-listing the headers somebody thought were at risk.
13. **Include spelling is split between quoted and angled forms for the same
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.
13. **Include spelling was split between quoted and angled forms.**
**Resolved.** Every in-project include in a public header uses
`#include <akgl/sibling.h>`, and `staticstring.h`'s `#include "string.h"` --
a relative-first lookup that reached the system header by accident -- is
`#include <string.h>`. `tests/*.c` still use `#include "testutil.h"`,
correctly: that one is test-local rather than installed.
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`.
14. **Empty parameter lists.** **Resolved.** `akgl_game_init`,
`akgl_game_update_fps`, `akgl_heap_init`, `akgl_heap_init_actor` and the
eight `akgl_registry_init*` functions declare and define `(void)`. Before
C23 `()` means "unspecified arguments" and suppresses argument checking, so
these were the entry points a caller could pass anything to.
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.
15. **`AKERR_NOIGNORE` was applied inconsistently at definition sites.**
**Resolved.** It is on the declarations, where it does its work, and on no
definition in `src/`.
### 3. Error-handling pattern
@@ -359,13 +369,14 @@ with no test to notice.
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.
parameter.** **Moot in 0.5.0**: `akgl_path_relative_from` is deleted (see
**Defects → Known and still open** item 4) and `akgl_path_relative_root` is
`static`, so `akgl_path_relative` is the only member of the family left and
there is nothing left to disagree with. It still takes `akgl_String *dst`
rather than the `**` form the rest of the library uses, which stays as
**item 41**'s question of naming and this one's question of shape -- but
with one function it is a decision to make when something needs it, not a
drift between siblings.
41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and the
`akgl_path_relative*` family use `dst`; everything else uses `dest`.
@@ -659,9 +670,13 @@ Each was found by a test written to assert correct behavior.
`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.
4. **`akgl_path_relative_from` is a stub that leaks.** **Deleted in 0.5.0.**
It claimed a heap string, never wrote `*dst`, and never released it, so 256
calls exhausted the string pool. It was declared in no header, called from
nowhere, and duplicated what `akgl_path_relative` already does. Fixing an
unfinished function nobody can reach is worse than deleting it; this also
closes item 40, which was about the output-parameter shape it disagreed with
the rest of the family on.
5. **`akgl_compare_sdl_surfaces` memcmps without checking geometry.**
`src/util.c:208` compares `s1->pitch * s1->h` bytes of `s2` without verifying
the surfaces share dimensions, pitch, or format.
@@ -682,10 +697,10 @@ Each was found by a test written to assert correct behavior.
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.
10. **`controller.h` declares functions that do not exist.** **Fixed in 0.5.0**;
the definitions carry the declared `akgl_controller_handle_*` names now. See
internal-consistency item 7, and `scripts/check_api_surface.sh`, which is
what stops this class of drift coming back.
11. **`akgl_controller_pushmap` and `akgl_controller_default` accept negative map
ids.** Both check `controlmapid >= AKGL_MAX_CONTROL_MAPS` but not
`controlmapid < 0`, so a negative id indexes before `GAME_ControlMaps`.