From 58b99e04777a9292c6ab68822769d52bdbf4eef3 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 07:23:57 -0400 Subject: [PATCH] Drop the savegame name-width aliases They were four #defines, one per table, each expanding to exactly one existing constant and used exactly once per side. That is indirection with no second consumer, and this repository's own rule is to abstract when the second consumer is real and not before. The reasoning behind them does not survive contact either. They were meant to let the on-disk format's field widths diverge from the object model's -- but raising AKGL_ACTOR_MAX_NAME_LENGTH is an ABI change, which bumps the version, and akgl_game_load refuses a save whose libversion does not match before it reads a single name table. The divergence they anticipated cannot happen quietly. The reader now names the same constant the writer does, per table, which is all the fix ever needed to be. What actually catches a disagreement is the EOF check at the end of akgl_game_load, and that works however the widths are spelled: reverting the spritesheet reader to the wrong constant still fails the roundtrip test with AKERR_IO. 25/25 pass, reindent --check clean. Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 31 ++++++++++++++++++---------- src/game.c | 59 ++++++++++++++++++++++++------------------------------ 2 files changed, 46 insertions(+), 44 deletions(-) diff --git a/TODO.md b/TODO.md index 983559e..0aa58a5 100644 --- a/TODO.md +++ b/TODO.md @@ -718,11 +718,10 @@ Each was found by a test written to assert correct behavior. header documented that as behaviour. It is `AKERR_OUTOFBOUNDS` now, and a negative count is refused too. 7. **Savegame name lengths disagree between writer and reader.** **Fixed in - 0.5.0.** Four `AKGL_GAME_SAVE_*_NAME_WIDTH` constants now drive both sides, - so the two cannot drift again. The writer used each object's own maximum name - length -- 512 for a spritesheet, which is a filename -- and the reader used - `AKGL_ACTOR_MAX_NAME_LENGTH` for all four. The other three are 128 as well, so - only the spritesheet table was wrong. + 0.5.0.** The reader names the same constant the writer does, per table -- + `AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` for spritesheets and so on. It used + `AKGL_ACTOR_MAX_NAME_LENGTH` for all four; the other three are 128 as well, + so only the spritesheet table was wrong, and that was enough. **The failure was worse than "cannot be read back", which is what made the test interesting.** The tables carry no length prefix and end at a zeroed @@ -731,16 +730,26 @@ Each was found by a test written to assert correct behavior. success with silently wrong maps. A test that only asserted the load succeeded passed against the broken reader. - So `akgl_game_load` now checks that the stream is at EOF once the four tables - are read. That turns a width disagreement into `AKERR_IO` instead of a - corruption, and it is the assertion `tests/game.c` actually hangs the test - on. The new roundtrip test registers a name in each of the four registries, - with a full-length one in the spritesheet registry, and against a mismatched - reader it fails with `AKERR_IO`. + So `akgl_game_load` checks the stream is at EOF once the four tables are + read. That is what turns a width disagreement into `AKERR_IO` instead of a + corruption, and it is the assertion `tests/game.c` hangs on. The new + roundtrip test registers a name in each of the four registries, with a + full-length one in the spritesheet registry, and fails with `AKERR_IO` + against a mismatched reader. That EOF check has to move when the objects themselves start being written; there is a comment at the site saying so. + A first pass at this introduced four `AKGL_GAME_SAVE_*_NAME_WIDTH` aliases, + one per table, on the reasoning that the on-disk format's widths are a + separate concern from the object model's. They were removed. Each expanded to + exactly one existing constant and had exactly one use per side, so they were + indirection with no second consumer -- and the divergence they anticipated + cannot happen quietly anyway: raising one of those lengths is an ABI change, + which bumps the version, and `akgl_game_load` refuses a save whose + `libversion` does not match before it reads a single table. What actually + guards the widths is the EOF check, which works however they are spelled. + 8. **Heap acquire functions are asymmetric.** `akgl_heap_next_string` increments `refcount`; `next_actor`, `next_sprite`, `next_spritesheet`, and `next_character` do not. `tests/heap.c` pins the current behavior and says so; diff --git a/src/game.c b/src/game.c index e30e08f..ffbfd3d 100644 --- a/src/game.c +++ b/src/game.c @@ -51,34 +51,27 @@ static akerr_ErrorContext *write_exact(const void *ptr, size_t size, size_t nmem return aksl_fwrite(ptr, size, nmemb, fp, &transferred); } -/** - * @brief Field widths of the four savegame name tables. +/* + * The four savegame name tables are written and read at each object's own + * maximum name length -- AKGL_ACTOR_MAX_NAME_LENGTH for actors, + * AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH for spritesheets, and so on. * - * The writer and the reader have to agree exactly. The tables carry no length - * prefix, so the reader finds each next entry by stepping this many bytes; get - * it wrong and every entry after the first is read out of the middle of its - * neighbour. - * - * They disagreed until 0.5.0. The writer used each object's own maximum name - * length -- 512 for a spritesheet, which is a filename -- and the reader used - * `AKGL_ACTOR_MAX_NAME_LENGTH` for all four. The other three happen to be 128 - * as well, so only the spritesheet table was wrong, and that was enough: a save - * containing any registered spritesheet could not be read back. The roundtrip - * test passed because empty registries write nothing but the zeroed sentinel. - * - * One constant per table, used on both sides, so the two cannot drift again. + * **The writer and the reader have to name the same constant per table.** The + * tables carry no length prefix, so the reader finds each next entry by + * stepping that many bytes. They disagreed until 0.5.0: the writer used each + * object's own length and the reader used AKGL_ACTOR_MAX_NAME_LENGTH for all + * four. The other three are 128 as well, so only the spritesheet table was + * wrong -- and that was enough. See the EOF check at the end of akgl_game_load, + * which is what turns a disagreement into an error instead of a corruption. */ -#define AKGL_GAME_SAVE_ACTOR_NAME_WIDTH AKGL_ACTOR_MAX_NAME_LENGTH -#define AKGL_GAME_SAVE_SPRITE_NAME_WIDTH AKGL_SPRITE_MAX_NAME_LENGTH -#define AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH -#define AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH AKGL_CHARACTER_MAX_NAME_LENGTH /** * @brief Widest name field any of the save tables writes. * - * The spritesheet table is the widest; the other three are half that or less. + * The spritesheet table is the widest at #AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH; + * the other three are name lengths half that or less. */ -#define AKGL_GAME_SAVE_MAX_NAME_FIELD AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH +#define AKGL_GAME_SAVE_MAX_NAME_FIELD AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH /** * @brief Fail the build if a table ever declares a field wider than the staging buffer. @@ -88,10 +81,10 @@ static akerr_ErrorContext *write_exact(const void *ptr, size_t size, size_t nmem * and turn back into the overread this buffer exists to prevent. */ typedef char akgl_game_save_name_field_fits[ - ((AKGL_GAME_SAVE_ACTOR_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && - (AKGL_GAME_SAVE_SPRITE_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && - (AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && - (AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD)) ? 1 : -1]; + ((AKGL_ACTOR_MAX_NAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && + (AKGL_SPRITE_MAX_NAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && + (AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && + (AKGL_CHARACTER_MAX_NAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD)) ? 1 : -1]; /** * @brief Write a registry key as a fixed-width, zero-padded field. @@ -321,7 +314,7 @@ static void save_actorname_iterator(void *userdata, SDL_PropertiesID props, cons PREPARE_ERROR(errctx); ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_ACTOR_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_ACTOR_MAX_NAME_LENGTH, fp)); actor = SDL_GetPointerProperty(props, name, NULL); CATCH(errctx, write_exact(&actor, 1, sizeof(akgl_Actor *), fp)); } CLEANUP { @@ -350,7 +343,7 @@ static void save_spritename_iterator(void *userdata, SDL_PropertiesID props, con ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); sprite = SDL_GetPointerProperty(props, name, NULL); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_SPRITE_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_SPRITE_MAX_NAME_LENGTH, fp)); CATCH(errctx, write_exact(&sprite, 1, sizeof(akgl_Sprite *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -382,7 +375,7 @@ static void save_spritesheetname_iterator(void *userdata, SDL_PropertiesID props ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); spritesheet = SDL_GetPointerProperty(props, name, NULL); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH, fp)); CATCH(errctx, write_exact(&spritesheet, 1, sizeof(akgl_SpriteSheet *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -410,7 +403,7 @@ static void save_charactername_iterator(void *userdata, SDL_PropertiesID props, ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); character = SDL_GetPointerProperty(props, name, NULL); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_CHARACTER_MAX_NAME_LENGTH, fp)); CATCH(errctx, write_exact(&character, 1, sizeof(akgl_Character *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -641,16 +634,16 @@ akerr_ErrorContext *akgl_game_load(char *fpath) memcpy((void *)&akgl_game, (void *)&savegame, sizeof(akgl_Game)); // Load actor name map actormap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, actormap, AKGL_GAME_SAVE_ACTOR_NAME_WIDTH, sizeof(akgl_Actor *), AKGL_REGISTRY_ACTOR)); + CATCH(errctx, load_objectnamemap(fp, actormap, AKGL_ACTOR_MAX_NAME_LENGTH, sizeof(akgl_Actor *), AKGL_REGISTRY_ACTOR)); // Load sprite name map spritemap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, spritemap, AKGL_GAME_SAVE_SPRITE_NAME_WIDTH, sizeof(akgl_Sprite *), AKGL_REGISTRY_SPRITE)); + CATCH(errctx, load_objectnamemap(fp, spritemap, AKGL_SPRITE_MAX_NAME_LENGTH, sizeof(akgl_Sprite *), AKGL_REGISTRY_SPRITE)); // Load spritesheet name map spritesheetmap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, spritesheetmap, AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH, sizeof(akgl_SpriteSheet *), AKGL_REGISTRY_SPRITESHEET)); + CATCH(errctx, load_objectnamemap(fp, spritesheetmap, AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH, sizeof(akgl_SpriteSheet *), AKGL_REGISTRY_SPRITESHEET)); // Load character name map charactermap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, charactermap, AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH, sizeof(akgl_Character *), AKGL_REGISTRY_CHARACTER)); + CATCH(errctx, load_objectnamemap(fp, charactermap, AKGL_CHARACTER_MAX_NAME_LENGTH, sizeof(akgl_Character *), AKGL_REGISTRY_CHARACTER)); // The four tables are the whole file, so the stream has to be at its end // now. Nothing else checks: the tables carry no length prefix and end at