Files
libakgl/src/character.c

288 lines
12 KiB
C
Raw Normal View History

/**
* @file character.c
* @brief Implements the character subsystem.
*/
#include <SDL3/SDL.h>
#include <SDL3_image/SDL_image.h>
#include <akerror.h>
Build clean under -Wall, with -Werror in CI only TODO.md item 37 said the cast sweep buys nothing until the build turns on the warnings those casts suppress. This is that precondition, and the argument turned out to be right with evidence: -Wall found three genuine signedness mismatches in sprite.c, where uint32_t width, height and speed were passed straight to akgl_get_json_integer_value(..., int *). A cast would have silenced all three. Fixing them properly also turned up that speed is scaled by a million into a 32-bit field, so anything past 4294 ms overflowed rather than being held. The other real finding was ten -Wstringop-truncation warnings, every one a fixed-width strncpy. Those leave a name field unterminated when the input fills it -- and those fields are registry keys, handed to strcmp and SDL property calls that do not stop at the field. Same overread class fixed twice already this release. All ten now use aksl_strncpy from akstdlib, which always terminates and never NUL-pads, bounded to size - 1 so an over-long name still truncates rather than being refused. staticstring.h documented the old strncpy semantics explicitly and has been rewritten to match. Two sites in game.c are deliberately left on strncpy: both stage into a pre-zeroed buffer for the savegame's fixed-width fields, where the NUL padding is part of the format. Neither is flagged. sprite.c also had a memcpy of a full 128-byte field from a NUL-terminated string, which reads past the end of any shorter name. Not flagged by anything; found while converting its neighbours. -Werror is an option, default OFF, on only in the cmake_build and performance CI jobs. libakgl is consumed with add_subdirectory -- akbasic does exactly that -- so a target-level -Werror turns every diagnostic a newer compiler invents into a broken build for somebody else. The warning set is not even stable across build types here: an -O2 build reports ten warnings that -O0 and -fsyntax-only do not, because they need the middle end. The two jobs that set it are one of each. -Wextra is deliberately not adopted. It adds 22 findings, 17 inherent to the design: 13 -Wunused-parameter from backend vtables and SDL callbacks that must match a signature, and 4 -Wimplicit-fallthrough from libakerror's own PROCESS/HANDLE/HANDLE_GROUP, which fall through by design. Filed upstream as deps/libakerror TODO item 8; the submodule bump carries it. deps/semver/semver.c is listed directly in add_library, so the target's PRIVATE options reach it. Exempted with -w so a future semver update cannot fail this build -- verified by forcing -Wextra -Werror and watching our files fail while semver compiled clean. Verified: RelWithDebInfo, Debug and coverage builds all clean under -Werror; -Werror actually fails on a planted unused variable; an embedded consumer with AKGL_WERROR unset still configures and builds. 25/25 pass, memcheck clean, reindent --check, check_api_surface and check_error_protocol clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 09:18:52 -04:00
#include <akstdlib.h>
#include <string.h>
#include <jansson.h>
#include <akgl/game.h>
#include <akgl/sprite.h>
#include <akgl/json_helpers.h>
#include <akgl/heap.h>
#include <akgl/registry.h>
#include <akgl/staticstring.h>
#include <akgl/iterator.h>
#include <akgl/util.h>
akerr_ErrorContext *akgl_character_initialize(akgl_Character *obj, char *name)
{
PREPARE_ERROR(errctx);
FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL akgl_Character reference");
FAIL_ZERO_RETURN(errctx, name, AKERR_NULLPOINTER, "NULL name string pointer");
memset(obj, 0x00, sizeof(akgl_Character));
Build clean under -Wall, with -Werror in CI only TODO.md item 37 said the cast sweep buys nothing until the build turns on the warnings those casts suppress. This is that precondition, and the argument turned out to be right with evidence: -Wall found three genuine signedness mismatches in sprite.c, where uint32_t width, height and speed were passed straight to akgl_get_json_integer_value(..., int *). A cast would have silenced all three. Fixing them properly also turned up that speed is scaled by a million into a 32-bit field, so anything past 4294 ms overflowed rather than being held. The other real finding was ten -Wstringop-truncation warnings, every one a fixed-width strncpy. Those leave a name field unterminated when the input fills it -- and those fields are registry keys, handed to strcmp and SDL property calls that do not stop at the field. Same overread class fixed twice already this release. All ten now use aksl_strncpy from akstdlib, which always terminates and never NUL-pads, bounded to size - 1 so an over-long name still truncates rather than being refused. staticstring.h documented the old strncpy semantics explicitly and has been rewritten to match. Two sites in game.c are deliberately left on strncpy: both stage into a pre-zeroed buffer for the savegame's fixed-width fields, where the NUL padding is part of the format. Neither is flagged. sprite.c also had a memcpy of a full 128-byte field from a NUL-terminated string, which reads past the end of any shorter name. Not flagged by anything; found while converting its neighbours. -Werror is an option, default OFF, on only in the cmake_build and performance CI jobs. libakgl is consumed with add_subdirectory -- akbasic does exactly that -- so a target-level -Werror turns every diagnostic a newer compiler invents into a broken build for somebody else. The warning set is not even stable across build types here: an -O2 build reports ten warnings that -O0 and -fsyntax-only do not, because they need the middle end. The two jobs that set it are one of each. -Wextra is deliberately not adopted. It adds 22 findings, 17 inherent to the design: 13 -Wunused-parameter from backend vtables and SDL callbacks that must match a signature, and 4 -Wimplicit-fallthrough from libakerror's own PROCESS/HANDLE/HANDLE_GROUP, which fall through by design. Filed upstream as deps/libakerror TODO item 8; the submodule bump carries it. deps/semver/semver.c is listed directly in add_library, so the target's PRIVATE options reach it. Exempted with -w so a future semver update cannot fail this build -- verified by forcing -Wextra -Werror and watching our files fail while semver compiled clean. Verified: RelWithDebInfo, Debug and coverage builds all clean under -Werror; -Werror actually fails on a planted unused variable; an embedded consumer with AKGL_WERROR unset still configures and builds. 25/25 pass, memcheck clean, reindent --check, check_api_surface and check_error_protocol clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 09:18:52 -04:00
// Always terminated: this is a registry key, and strncpy at exactly the
// field width leaves an over-long name unterminated.
PASS(errctx, aksl_strncpy(obj->name, sizeof(obj->name), name, sizeof(obj->name) - 1));
obj->state_sprites = SDL_CreateProperties();
FAIL_ZERO_RETURN(errctx, obj->state_sprites, AKERR_NULLPOINTER, "Unable to initialize SDL_PropertiesID for character state map");
obj->sprite_add = &akgl_character_sprite_add;
obj->sprite_get = &akgl_character_sprite_get;
FAIL_ZERO_RETURN(
errctx,
SDL_SetPointerProperty(AKGL_REGISTRY_CHARACTER, name, (void *)obj),
AKERR_KEY,
"Unable to add character to registry");
obj->refcount += 1;
SUCCEED_RETURN(errctx);
}
akerr_ErrorContext *akgl_character_sprite_add(akgl_Character *basechar, akgl_Sprite *ref, int state)
{
PREPARE_ERROR(errctx);
char stateval[32];
Give back every pooled string and sprite reference the loaders take Closes Defects items 20, 22, 23 and 29, and the residual of item 38. The tilemap load leak was five pooled strings per load; the property-lookup fix in an earlier commit took it to two, and the last two were each a claim with no matching release -- the string every layer's `type` was read into, and the dirname the map's relative paths resolve against. tests/tilemap.c asserts the pool is exactly where it started after one load/release cycle and after 64. Finding those two was a matter of dumping the contents of every still-claimed slot after a cycle rather than reading the code again; 'tilelayer' and an assets directory named themselves immediately. Same file, same class, fixed with it: akgl_tilemap_load_layer_objects released its scratch string after reading each object's name and then kept using the slot, because akgl_get_json_string_value reuses a non-NULL destination without taking another reference. The slot was free while still live, so any other claim could have been handed it. akgl_character_sprite_add wrote over an existing binding without releasing the sprite it displaced, so a character that rebinds a state while alive leaked a sprite slot per rebind -- teardown only gives back what the map holds at the end. The new reference is taken before the write and given back if the write fails, so there is no window where a sprite is bound with nothing behind it. The write was unchecked too. Three failure-path leaks moved into CLEANUP blocks: akgl_render_2d_init's two pooled strings, akgl_controller_open_gamepads' enumeration array, and akgl_text_rendertextat's surface and texture -- the last being a leak per frame on a HUD line. akgl_text_unloadallfonts() closes every font in the registry and destroys it, which is what item 38 left open. Deliberately not a whole akgl_game_shutdown: tearing down the mixer, SDL_ttf and SDL in the right order is a design question, and this is the part that was simply missing. It is a new public symbol, which 0.5.0 already covers -- this release has not shipped. Every fix has a test that fails against the old code. 25/25 pass, memcheck clean, reindent --check, check_api_surface and check_error_protocol all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 00:44:50 -04:00
akgl_Sprite *displaced = NULL;
FAIL_ZERO_RETURN(errctx, basechar, AKERR_NULLPOINTER, "NULL character reference");
FAIL_ZERO_RETURN(errctx, ref, AKERR_NULLPOINTER, "NULL sprite reference");
memset(&stateval, 0x00, 32);
SDL_itoa(state, (char *)&stateval, 10);
Give back every pooled string and sprite reference the loaders take Closes Defects items 20, 22, 23 and 29, and the residual of item 38. The tilemap load leak was five pooled strings per load; the property-lookup fix in an earlier commit took it to two, and the last two were each a claim with no matching release -- the string every layer's `type` was read into, and the dirname the map's relative paths resolve against. tests/tilemap.c asserts the pool is exactly where it started after one load/release cycle and after 64. Finding those two was a matter of dumping the contents of every still-claimed slot after a cycle rather than reading the code again; 'tilelayer' and an assets directory named themselves immediately. Same file, same class, fixed with it: akgl_tilemap_load_layer_objects released its scratch string after reading each object's name and then kept using the slot, because akgl_get_json_string_value reuses a non-NULL destination without taking another reference. The slot was free while still live, so any other claim could have been handed it. akgl_character_sprite_add wrote over an existing binding without releasing the sprite it displaced, so a character that rebinds a state while alive leaked a sprite slot per rebind -- teardown only gives back what the map holds at the end. The new reference is taken before the write and given back if the write fails, so there is no window where a sprite is bound with nothing behind it. The write was unchecked too. Three failure-path leaks moved into CLEANUP blocks: akgl_render_2d_init's two pooled strings, akgl_controller_open_gamepads' enumeration array, and akgl_text_rendertextat's surface and texture -- the last being a leak per frame on a HUD line. akgl_text_unloadallfonts() closes every font in the registry and destroys it, which is what item 38 left open. Deliberately not a whole akgl_game_shutdown: tearing down the mixer, SDL_ttf and SDL in the right order is a design question, and this is the part that was simply missing. It is a new public symbol, which 0.5.0 already covers -- this release has not shipped. Every fix has a test that fails against the old code. 25/25 pass, memcheck clean, reindent --check, check_api_surface and check_error_protocol all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 00:44:50 -04:00
// Whatever this state was bound to is losing its binding, and the reference
// taken for that binding with it. Without this a character that rebinds a
// state while alive leaks one sprite slot per rebind -- releasing at
// teardown only gives back whatever the map happens to hold at the end.
displaced = (akgl_Sprite *)SDL_GetPointerProperty(basechar->state_sprites, (char *)&stateval, NULL);
// Take the new reference before anything can fail, so a failed write cannot
// leave the sprite bound with no reference behind it.
ref->refcount += 1;
Give back every pooled string and sprite reference the loaders take Closes Defects items 20, 22, 23 and 29, and the residual of item 38. The tilemap load leak was five pooled strings per load; the property-lookup fix in an earlier commit took it to two, and the last two were each a claim with no matching release -- the string every layer's `type` was read into, and the dirname the map's relative paths resolve against. tests/tilemap.c asserts the pool is exactly where it started after one load/release cycle and after 64. Finding those two was a matter of dumping the contents of every still-claimed slot after a cycle rather than reading the code again; 'tilelayer' and an assets directory named themselves immediately. Same file, same class, fixed with it: akgl_tilemap_load_layer_objects released its scratch string after reading each object's name and then kept using the slot, because akgl_get_json_string_value reuses a non-NULL destination without taking another reference. The slot was free while still live, so any other claim could have been handed it. akgl_character_sprite_add wrote over an existing binding without releasing the sprite it displaced, so a character that rebinds a state while alive leaked a sprite slot per rebind -- teardown only gives back what the map holds at the end. The new reference is taken before the write and given back if the write fails, so there is no window where a sprite is bound with nothing behind it. The write was unchecked too. Three failure-path leaks moved into CLEANUP blocks: akgl_render_2d_init's two pooled strings, akgl_controller_open_gamepads' enumeration array, and akgl_text_rendertextat's surface and texture -- the last being a leak per frame on a HUD line. akgl_text_unloadallfonts() closes every font in the registry and destroys it, which is what item 38 left open. Deliberately not a whole akgl_game_shutdown: tearing down the mixer, SDL_ttf and SDL in the right order is a design question, and this is the part that was simply missing. It is a new public symbol, which 0.5.0 already covers -- this release has not shipped. Every fix has a test that fails against the old code. 25/25 pass, memcheck clean, reindent --check, check_api_surface and check_error_protocol all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 00:44:50 -04:00
if ( SDL_SetPointerProperty(basechar->state_sprites, (char *)&stateval, ref) == false ) {
ref->refcount -= 1;
FAIL_RETURN(
errctx,
AKERR_KEY,
"Unable to bind sprite %s to character %s for state %d: %s",
(char *)&ref->name,
(char *)&basechar->name,
state,
SDL_GetError());
}
if ( (displaced != NULL) && (displaced != ref) ) {
PASS(errctx, akgl_heap_release_sprite(displaced));
}
SDL_Log("Added sprite %s to character %s for state %b", (char *)&ref->name, (char *)&basechar->name, state);
SUCCEED_RETURN(errctx);
}
akerr_ErrorContext *akgl_character_sprite_get(akgl_Character *basechar, int state, akgl_Sprite **dest)
{
PREPARE_ERROR(errctx);
char stateval[32];
FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL pointer to sprite pointer (**dest)");
FAIL_ZERO_RETURN(errctx, basechar, AKERR_NULLPOINTER, "NULL character reference");
memset(&stateval, 0x00, 32);
SDL_itoa(state, (char *)&stateval, 10);
*dest = (akgl_Sprite *)SDL_GetPointerProperty(basechar->state_sprites, (char *)&stateval, NULL);
FAIL_ZERO_RETURN(errctx, *dest, AKERR_KEY, "Sprite for state %d (%b) not found in the character's registry", state, state);
SUCCEED_RETURN(errctx);
}
// SDL iterator so we can't return error information here, void only
// this means we don't have anywhere to send exceptions up to, so if we hit an error, we log and exit(1) here
Give every exported function a declaration, and check that it stays that way 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>
2026-07-31 23:44:38 -04:00
void akgl_character_state_sprites_iterate(void *userdata, SDL_PropertiesID props, const char *name)
{
PREPARE_ERROR(errctx);
akgl_Sprite *spriteptr;
akgl_Iterator *opflags = (akgl_Iterator *)userdata;
ATTEMPT {
FAIL_ZERO_BREAK(errctx, opflags, AKERR_NULLPOINTER, "Character state sprite iterator received null iterator op pointer");
FAIL_ZERO_BREAK(errctx, name, AKERR_NULLPOINTER, "Character state sprite iterator received null sprite name");
Give every exported function a declaration, and check that it stays that way 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>
2026-07-31 23:44:38 -04:00
spriteptr = (akgl_Sprite *)SDL_GetPointerProperty(props, name, NULL);
FAIL_ZERO_BREAK(errctx, spriteptr, AKERR_NULLPOINTER, "Character state sprite for %s not found", name);
if ( AKGL_BITMASK_HAS(opflags->flags, AKGL_ITERATOR_OP_RELEASE) ) {
CATCH(errctx, akgl_heap_release_sprite(spriteptr));
}
} CLEANUP {
} PROCESS(errctx) {
} FINISH_NORETURN(errctx);
}
/**
Document what the functions actually do instead of that they can fail The Doxygen comments were generated from the declarations, so 217 @throws lines across 21 headers read "When the corresponding validation or operation fails" and told a caller nothing beyond the status name. The @param lines were the same shape: every output was "Output destination populated by the function", every instance "Object to initialize, inspect, or modify". Rewritten against the implementations, following the pattern libakstdlib already uses: - @throws names the condition. akgl_sprite_load_json separated AKERR_KEY (absent) from AKERR_TYPE (present, wrong type) from AKERR_OUTOFBOUNDS (filename too long, or array indexed past its end), and gained AKGL_ERR_SDL and AKGL_ERR_HEAP, which it raises and never declared. - Parameters say whether they are required, what a NULL means, and what is written on a failure path. Where an argument is not checked, the doc says so: akgl_heap_next_actor's dest is a crash on NULL, not an error, and akgl_render_2d_frame_start dereferences self before testing it. - The conventions move up to the file blocks so the per-function docs stay short. json_helpers.h states once that absence is an error here and that json_t * results are borrowed; heap.h explains the pool model and the acquire asymmetry; physics.h carries the thrust/environmental/velocity table. - Struct fields, enum values, macros and exported globals are documented, including the dead ones - sprite_w/sprite_h, movetimer, p_scale and timer_gravity are read by nothing, and say so. Also fixes ten comments in error.h and audio.h that opened with /** rather than /**<, so Doxygen attached them to the following entity and rendered the text as part of the macro's value. Verified against the generated HTML. Comments only - no declaration changed. Doxygen builds clean under WARN_AS_ERROR, scripts/reindent.sh --check passes, 19/19 suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 11:02:20 -04:00
* @brief OR a JSON array of actor-state names together into one bitmask.
*
* This is what lets a character JSON say `["FACE_LEFT", "MOVING_LEFT"]` rather
* than a number: each name is looked up in #AKGL_REGISTRY_ACTOR_STATE_STRINGS
* and OR-ed in. A name that is not a known state is an error rather than being
* skipped -- a typo in a state name would otherwise bind a sprite to the wrong
* combination and show up as art that never appears.
*
* @param states JSON array of state-name strings. Required. An empty array is
* legal and leaves @p dest as it was.
* @param dest Receives the OR of every named bit. Required in practice, and
* **not** checked -- the guard that should test it tests @p states
* a second time instead, so a `NULL` here is a crash. It is OR-ed
* into rather than assigned, so the caller must zero it first.
* @return `NULL` on success, otherwise an error context owned by the caller.
Document what the functions actually do instead of that they can fail The Doxygen comments were generated from the declarations, so 217 @throws lines across 21 headers read "When the corresponding validation or operation fails" and told a caller nothing beyond the status name. The @param lines were the same shape: every output was "Output destination populated by the function", every instance "Object to initialize, inspect, or modify". Rewritten against the implementations, following the pattern libakstdlib already uses: - @throws names the condition. akgl_sprite_load_json separated AKERR_KEY (absent) from AKERR_TYPE (present, wrong type) from AKERR_OUTOFBOUNDS (filename too long, or array indexed past its end), and gained AKGL_ERR_SDL and AKGL_ERR_HEAP, which it raises and never declared. - Parameters say whether they are required, what a NULL means, and what is written on a failure path. Where an argument is not checked, the doc says so: akgl_heap_next_actor's dest is a crash on NULL, not an error, and akgl_render_2d_frame_start dereferences self before testing it. - The conventions move up to the file blocks so the per-function docs stay short. json_helpers.h states once that absence is an error here and that json_t * results are borrowed; heap.h explains the pool model and the acquire asymmetry; physics.h carries the thrust/environmental/velocity table. - Struct fields, enum values, macros and exported globals are documented, including the dead ones - sprite_w/sprite_h, movetimer, p_scale and timer_gravity are read by nothing, and say so. Also fixes ten comments in error.h and audio.h that opened with /** rather than /**<, so Doxygen attached them to the following entity and rendered the text as part of the macro's value. Verified against the generated HTML. Comments only - no declaration changed. Doxygen builds clean under WARN_AS_ERROR, scripts/reindent.sh --check passes, 19/19 suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 11:02:20 -04:00
* @throws AKERR_NULLPOINTER If @p states is `NULL`.
* @throws AKERR_KEY If a name is not in the actor-state registry. The message
* quotes it. Note that bits 11 and 12 are unreachable by name; see
* akgl_registry_init_actor_state_strings.
* @throws AKERR_TYPE If an array element is not a string.
* @throws AKERR_OUTOFBOUNDS If the array is indexed past its end.
* @throws AKGL_ERR_HEAP If the string pool is exhausted.
*/
Give every exported function a declaration, and check that it stays that way 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>
2026-07-31 23:44:38 -04:00
static akerr_ErrorContext *character_load_json_state_int_from_strings(json_t *states, int *dest)
{
int i = 0;
long newstate = 0;
akgl_String *tmpstring = NULL;
PREPARE_ERROR(errctx);
FAIL_ZERO_RETURN(errctx, states, AKERR_NULLPOINTER, "NULL states array");
Make savegames, optional array elements, empty text and coverage work Closes Known-and-still-open items 7 and 13, and Defects items 18, 25 and 27. The savegame name tables carry no length prefix, so writer and reader have to agree on a field width exactly. The writer used each object's own maximum name length -- 512 for a spritesheet, a filename -- and the reader used AKGL_ACTOR_MAX_NAME_LENGTH for all four. Four AKGL_GAME_SAVE_*_NAME_WIDTH constants drive both sides now. The failure turned out to be worse than "cannot be read back": a reader stepping the wrong width does not run off anything, it finds a run of zeros inside an entry, stops early, and reports success with silently wrong maps. A test asserting only that the load succeeded passed against the broken reader. So akgl_game_load checks it is at EOF once the tables are read, which turns a width disagreement into AKERR_IO instead of a corruption. That is the assertion the new roundtrip test -- the first with all four registries populated -- hangs on. akgl_get_json_with_default gains a third HANDLE_GROUP for AKERR_OUTOFBOUNDS, which is what the array index accessors report, so "this element is optional" works for an array element and not only for an object member. The arm goes above the one holding the memcpy: HANDLE_GROUP emits no break and every arm falls into that body. That test needed a second attempt. with_default returns *the context it was given* when it does not handle the status, so TEST_EXPECT_OK -- which releases whatever the statement returns -- double-released it against a CLEANUP block that released it too, and a double-released context corrupts the failure rather than reporting it. The first draft passed against the unfixed library. akgl_text_rendertextat returns success without rasterizing for the empty string, matching akgl_text_measure, which has always accepted it. The check sits after the font and backend guards, so drawing nothing still refuses what drawing something refuses. tests/text.c had this case written and unasserted waiting for the two halves of the header to agree. character_load_json_state_int_from_strings guards dest rather than testing states twice. Not asserted: the function is static with one call site that passes a real pointer, so the guard cannot fire, and reaching it from a test would mean giving it external linkage purely for that. Both gcovr invocations take the build tree as an explicit positional search path. gcovr searches --root when given none, which is the source directory, where build trees live; --object-directory does not narrow it. Verified by building two instrumented trees with a source edit between them: the old invocation fails with "Got function write_exact on multiple lines: 46, 48" and exits 64, the new one exits 0 and the full coverage run passes with the stale tree still present. 25/25 pass, reindent --check, check_api_surface and check_error_protocol clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 07:18:31 -04:00
FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination integer");
ATTEMPT {
CATCH(errctx, akgl_heap_next_string(&tmpstring));
for ( i = 0; i < json_array_size((json_t *)states) ; i++ ) {
CATCH(errctx, akgl_get_json_array_index_string(states, i, &tmpstring));
newstate = (long)SDL_GetNumberProperty(AKGL_REGISTRY_ACTOR_STATE_STRINGS, (char *)&tmpstring->data, 0);
FAIL_ZERO_BREAK(errctx, newstate, AKERR_KEY, "Unknown actor state %s", (char *)&tmpstring->data);
*dest = (*dest | (int)(newstate));
}
} CLEANUP {
IGNORE(akgl_heap_release_string(tmpstring));
} PROCESS(errctx) {
} FINISH(errctx, true);
SUCCEED_RETURN(errctx);
}
/**
Document what the functions actually do instead of that they can fail The Doxygen comments were generated from the declarations, so 217 @throws lines across 21 headers read "When the corresponding validation or operation fails" and told a caller nothing beyond the status name. The @param lines were the same shape: every output was "Output destination populated by the function", every instance "Object to initialize, inspect, or modify". Rewritten against the implementations, following the pattern libakstdlib already uses: - @throws names the condition. akgl_sprite_load_json separated AKERR_KEY (absent) from AKERR_TYPE (present, wrong type) from AKERR_OUTOFBOUNDS (filename too long, or array indexed past its end), and gained AKGL_ERR_SDL and AKGL_ERR_HEAP, which it raises and never declared. - Parameters say whether they are required, what a NULL means, and what is written on a failure path. Where an argument is not checked, the doc says so: akgl_heap_next_actor's dest is a crash on NULL, not an error, and akgl_render_2d_frame_start dereferences self before testing it. - The conventions move up to the file blocks so the per-function docs stay short. json_helpers.h states once that absence is an error here and that json_t * results are borrowed; heap.h explains the pool model and the acquire asymmetry; physics.h carries the thrust/environmental/velocity table. - Struct fields, enum values, macros and exported globals are documented, including the dead ones - sprite_w/sprite_h, movetimer, p_scale and timer_gravity are read by nothing, and say so. Also fixes ten comments in error.h and audio.h that opened with /** rather than /**<, so Doxygen attached them to the following entity and rendered the text as part of the macro's value. Verified against the generated HTML. Comments only - no declaration changed. Doxygen builds clean under WARN_AS_ERROR, scripts/reindent.sh --check passes, 19/19 suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 11:02:20 -04:00
* @brief Name a character and build its state-to-sprite map from a JSON document.
*
* The half of akgl_character_load_json that deals with identity and sprites; the
* caller handles the numeric fields afterwards. Reads `name`, initializes the
* character with it, then walks `sprite_mappings` binding each named sprite to
* the bitmask its `state` array adds up to.
*
* Every sprite named must already be in #AKGL_REGISTRY_SPRITE.
*
* @param json The parsed character document. Required in practice; not checked,
* and passed straight to accessors that report it.
* @param obj The pooled character to fill in. Required, unchecked -- it is
* handed to akgl_character_initialize, which does check it.
* @return `NULL` on success, otherwise an error context owned by the caller.
Document what the functions actually do instead of that they can fail The Doxygen comments were generated from the declarations, so 217 @throws lines across 21 headers read "When the corresponding validation or operation fails" and told a caller nothing beyond the status name. The @param lines were the same shape: every output was "Output destination populated by the function", every instance "Object to initialize, inspect, or modify". Rewritten against the implementations, following the pattern libakstdlib already uses: - @throws names the condition. akgl_sprite_load_json separated AKERR_KEY (absent) from AKERR_TYPE (present, wrong type) from AKERR_OUTOFBOUNDS (filename too long, or array indexed past its end), and gained AKGL_ERR_SDL and AKGL_ERR_HEAP, which it raises and never declared. - Parameters say whether they are required, what a NULL means, and what is written on a failure path. Where an argument is not checked, the doc says so: akgl_heap_next_actor's dest is a crash on NULL, not an error, and akgl_render_2d_frame_start dereferences self before testing it. - The conventions move up to the file blocks so the per-function docs stay short. json_helpers.h states once that absence is an error here and that json_t * results are borrowed; heap.h explains the pool model and the acquire asymmetry; physics.h carries the thrust/environmental/velocity table. - Struct fields, enum values, macros and exported globals are documented, including the dead ones - sprite_w/sprite_h, movetimer, p_scale and timer_gravity are read by nothing, and say so. Also fixes ten comments in error.h and audio.h that opened with /** rather than /**<, so Doxygen attached them to the following entity and rendered the text as part of the macro's value. Verified against the generated HTML. Comments only - no declaration changed. Doxygen builds clean under WARN_AS_ERROR, scripts/reindent.sh --check passes, 19/19 suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 11:02:20 -04:00
* @throws AKERR_NULLPOINTER If a mapping names a sprite that is not registered.
* The message names the character, the state, and the sprite.
* @throws AKERR_KEY If `name`, `sprite_mappings`, or a mapping's `sprite` or
* `state` is absent, or if a state name is unknown.
* @throws AKERR_TYPE If one of those keys has the wrong JSON type.
* @throws AKERR_OUTOFBOUNDS If an array is indexed past its end.
* @throws AKGL_ERR_HEAP If the string pool is exhausted.
*/
Give every exported function a declaration, and check that it stays that way 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>
2026-07-31 23:44:38 -04:00
static akerr_ErrorContext *character_load_json_inner(json_t *json, akgl_Character *obj)
{
PREPARE_ERROR(errctx);
json_t *mappings = NULL;
json_t *curmapping = NULL;
json_t *statearray = NULL;
akgl_Sprite *spriteptr = NULL;
int i = 0;
akgl_String *tmpstr = NULL;
akgl_String *tmpstr2 = NULL;
int stateval = 0;
ATTEMPT {
CATCH(errctx, akgl_get_json_string_value((json_t *)json, "name", &tmpstr));
CATCH(errctx, akgl_character_initialize((akgl_Character *)obj, tmpstr->data));
CATCH(errctx, akgl_get_json_array_value((json_t *)json, "sprite_mappings", &mappings));
for ( i = 0; i < json_array_size((json_t *)mappings) ; i++ ) {
stateval = 0;
CATCH(errctx, akgl_get_json_array_index_object((json_t *)mappings, i, &curmapping));
CATCH(errctx, akgl_get_json_string_value((json_t *)curmapping, "sprite", &tmpstr));
spriteptr = (akgl_Sprite *)SDL_GetPointerProperty(
AKGL_REGISTRY_SPRITE,
tmpstr->data,
NULL
);
CATCH(errctx, akgl_get_json_string_value((json_t *)json, "name", &tmpstr2));
CATCH(errctx, akgl_get_json_array_value((json_t *)curmapping, "state", &statearray));
Give every exported function a declaration, and check that it stays that way 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>
2026-07-31 23:44:38 -04:00
CATCH(errctx, character_load_json_state_int_from_strings(statearray, &stateval));
CATCH(errctx, akgl_get_json_string_value((json_t *)curmapping, "sprite", &tmpstr));
FAIL_ZERO_BREAK(
errctx,
spriteptr,
AKERR_NULLPOINTER,
2025-08-08 22:39:48 -04:00
"Character %s for state %b references sprite %s but not found in the registry",
tmpstr2->data,
stateval,
tmpstr->data
);
CATCH(errctx, akgl_character_sprite_add((akgl_Character *)obj, (akgl_Sprite *)spriteptr, stateval));
}
} CLEANUP {
if ( tmpstr != NULL ) {
IGNORE(akgl_heap_release_string(tmpstr));
}
if ( tmpstr2 != NULL ) {
IGNORE(akgl_heap_release_string(tmpstr2));
}
} PROCESS(errctx) {
} FINISH(errctx, true);
SUCCEED_RETURN(errctx);
}
akerr_ErrorContext *akgl_character_load_json(char *filename)
{
PREPARE_ERROR(errctx);
Fix every memory defect the checker found, and bump to 0.4.0 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>
2026-07-31 15:49:11 -04:00
json_t *json = NULL;
json_error_t error;
akgl_Character *obj = NULL;
//akgl_String *tmpstr = NULL;
FAIL_ZERO_RETURN(errctx, filename, AKERR_NULLPOINTER, "Received null filename");
ATTEMPT {
CATCH(errctx, akgl_heap_next_character(&obj));
//CATCH(errctx, akgl_heap_next_string(&tmpstr));
//CATCH(errctx, akgl_string_initialize(tmpstr, NULL));
json = (json_t *)json_load_file(filename, 0, &error);
FAIL_ZERO_BREAK(
errctx,
json,
AKERR_NULLPOINTER,
"Error while loading character from %s on line %d: %s", filename, error.line, error.text
);
Give every exported function a declaration, and check that it stays that way 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>
2026-07-31 23:44:38 -04:00
CATCH(errctx, character_load_json_inner(json, obj));
CATCH(errctx, akgl_get_json_integer_value(json, "speedtime", (int *)&obj->speedtime));
Namespace every exported symbol, and bump to 0.5.0 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>
2026-07-31 23:32:21 -04:00
obj->speedtime = obj->speedtime * AKGL_TIME_ONEMS_NS;
CATCH(errctx, akgl_get_json_number_value(json, "speed_x", &obj->sx));
CATCH(errctx, akgl_get_json_number_value(json, "speed_y", &obj->sy));
CATCH(errctx, akgl_get_json_number_value(json, "acceleration_x", &obj->ax));
CATCH(errctx, akgl_get_json_number_value(json, "acceleration_y", &obj->ay));
} CLEANUP {
//IGNORE(akgl_heap_release_string(tmpstr));
Fix every memory defect the checker found, and bump to 0.4.0 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>
2026-07-31 15:49:11 -04:00
// The character keeps nothing that points into the document -- every
// field above is copied out of it -- so the whole parsed tree goes back
// here, on the success path as well as the failure one.
if ( json != NULL ) {
json_decref(json);
json = NULL;
}
if ( errctx != NULL ) {
if ( errctx->status != 0 ) {
IGNORE(akgl_heap_release_character(obj));
}
}
} PROCESS(errctx) {
} FINISH(errctx, true);
SDL_Log("Character %s loaded from %s", obj->name, filename);
SUCCEED_RETURN(errctx);
}