diff --git a/CMakeLists.txt b/CMakeLists.txt index 71d8586..0afa568 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -409,12 +409,29 @@ if(AKGL_COVERAGE) set(AKGL_COVERAGE_DIR "${CMAKE_CURRENT_BINARY_DIR}/coverage") file(MAKE_DIRECTORY "${AKGL_COVERAGE_DIR}") + # Both gcovr invocations take the build tree as an explicit positional search + # path, and neither passes --object-directory. + # + # gcovr looks for .gcda/.gcno under its search paths, and with none given it + # searches --root -- the source directory, which is where developers keep + # their build trees. --object-directory does *not* narrow that: per gcovr's + # own help it only identifies "the path between gcda files and the directory + # where the compiler was originally run". So every instrumented tree left + # inside the source directory was folded into the report alongside the one + # being measured, and with two trees describing different line numbers for the + # same function, coverage_reset failed before any test ran: + # + # AssertionError: Got function akgl_game_lowfps on multiple lines: 45, 46 + # + # The `build*/` entry in .gitignore hides those trees from git status, which + # makes the state easy to get into and hard to notice. + add_test( NAME coverage_reset COMMAND ${GCOVR_EXECUTABLE} --root "${CMAKE_CURRENT_SOURCE_DIR}" - --object-directory "${CMAKE_CURRENT_BINARY_DIR}" --delete + "${CMAKE_CURRENT_BINARY_DIR}" ) set_tests_properties(coverage_reset PROPERTIES FIXTURES_SETUP akgl_coverage) # Any suite missing from this list runs outside the fixture and has its @@ -430,11 +447,11 @@ if(AKGL_COVERAGE) NAME coverage_report COMMAND ${GCOVR_EXECUTABLE} --root "${CMAKE_CURRENT_SOURCE_DIR}" - --object-directory "${CMAKE_CURRENT_BINARY_DIR}" --filter "${CMAKE_CURRENT_SOURCE_DIR}/src/" --xml-pretty --xml "${AKGL_COVERAGE_DIR}/coverage.xml" --html-details "${AKGL_COVERAGE_DIR}/index.html" + "${CMAKE_CURRENT_BINARY_DIR}" ) set_tests_properties( coverage_report diff --git a/TODO.md b/TODO.md index 741de00..983559e 100644 --- a/TODO.md +++ b/TODO.md @@ -717,13 +717,30 @@ Each was found by a test written to assert correct behavior. read past the end of one pool slot and wrote past the end of another. The 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.** - `akgl_game_save_actors` writes spritesheet names at - `AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` (512) and character names at - `AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH` (128), but `akgl_game_load` reads - every table at `AKGL_ACTOR_MAX_NAME_LENGTH` (128). A save with any registered - spritesheet cannot be read back. The current roundtrip test passes only - because empty registries write nothing but zeroed sentinels. +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. + + **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 + sentinel, so a reader stepping the wrong width does not run off anything -- + it finds a run of zeros somewhere inside an entry, stops early, and reports + 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`. + + That EOF check has to move when the objects themselves start being written; + there is a comment at the site saying so. + 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; @@ -778,40 +795,27 @@ Each was found by a test written to assert correct behavior. it so a future regeneration shows no spurious diff. 13. **A stale build tree in the source directory breaks the coverage run.** - `CMakeLists.txt:208` and `CMakeLists.txt:223` pass - `--root "${CMAKE_CURRENT_SOURCE_DIR}"` to gcovr, and gcovr searches for - `.gcda`/`.gcno` files under the root. The `--object-directory` argument on - the line below each does *not* narrow that search: per `gcovr --help` it - only identifies "the path between gcda files and the directory where the - compiler was originally run". So every instrumented build tree left inside - the source directory is folded into the report alongside the one actually - being measured. + **Fixed in 0.5.0.** Both `gcovr` invocations take the build tree as an + explicit positional search path and neither passes `--object-directory`. - With a `build-coverage/` from an earlier session still present, a freshly - configured `-DAKGL_COVERAGE=ON` tree fails in `coverage_reset`, before any - test runs: + gcovr searches for `.gcda`/`.gcno` under its search paths, and with none + given it searches `--root` -- the source directory, which is where + developers keep their build trees. `--object-directory` does not narrow + that; per gcovr's own help it only identifies "the path between gcda files + and the directory where the compiler was originally run". - AssertionError: Got function akgl_game_lowfps on multiple lines: 45, 46. + **Verified by reproducing it.** Two instrumented trees were built inside the + source directory with a source edit between them, so they described + different line numbers for the same functions. The old invocation fails with + `Got function write_exact on multiple lines: 46, 48` and exits 64; the new + one exits 0 and the whole 25-test coverage run passes with the stale tree + still sitting there. - 45 and 46 are that function's line numbers before and after an unrelated - `#include` was added to `src/game.c`: gcovr found 18 stale `.gcno` files - describing the old layout, merged them with the current ones, and could not - reconcile the two. Moving `build-coverage/` aside makes the same tree pass - 18/18. Verified with gcovr 7.0. - - The `build*/` entry in `.gitignore` hides these trees from `git status`, - which makes the state easier to get into and no easier to notice. - - A second, smaller version of the same thing: rebuilding an existing - coverage tree after editing a test leaves `.gcda` files describing the old - object layout, and `coverage_reset` -- whose whole job is to delete them -- - fails with `GCOV returncode was 5` before it gets the chance. `find - build-coverage -name '*.gcda' -delete` clears it. Observed with gcovr 7.0. - - Fix: pass the build tree to gcovr as an explicit search path instead of - letting it default to `--root`, so only the tree under measurement is - considered. Touches the two `add_test` blocks at `CMakeLists.txt:204-211` - and `CMakeLists.txt:220-229`; nothing outside the `AKGL_COVERAGE` branch. + The second, smaller version of the same thing -- rebuilding a coverage tree + after editing a test leaves `.gcda` files describing the old object layout, + and `coverage_reset` fails with `GCOV returncode was 5` before it can delete + them -- is unchanged. `find build-coverage -name '*.gcda' -delete` clears + it. That one is gcov's, not gcovr's search path. 14. **22 public symbols shipped without a version or soname bump.** 42b60f7 added `akgl_draw_point`, `_line`, `_rect`, `_filled_rect`, `_circle`, `_flood_fill`, @@ -888,18 +892,21 @@ without coming here first. Ordered by blast radius. and `akgl_TilemapObject` is not small. 18. **`akgl_get_json_with_default` defaults on a status the array accessors never - raise.** `src/json_helpers.c:164-165` handles `AKERR_KEY` and `AKERR_INDEX`, - but `akgl_get_json_array_index_object`, `_integer` and `_string` all report - a short array as `AKERR_OUTOFBOUNDS`. So the "this element is optional" form - silently does not work for an array — the error propagates instead of being - replaced by the default. Only the object accessors, which do raise - `AKERR_KEY`, are actually served by this function today. Nothing in tree - passes an array accessor's error here, which is why it has not been noticed. + raise.** **Fixed in 0.5.0**: a third `HANDLE_GROUP(e, AKERR_OUTOFBOUNDS)`, + placed *above* the arm holding the `memcpy`, since `HANDLE_GROUP` emits no + `break` and every arm falls into that body. - Fix: add a third `HANDLE_GROUP(err, AKERR_OUTOFBOUNDS)`. Note that the - existing two rely on `HANDLE_GROUP` not emitting a `break`, so the `KEY` - case falls through into the `INDEX` body — a third arm has to go *above* the - one holding the `memcpy`, not below it. Touches `src/json_helpers.c:164-167`. + `tests/json_helpers.c` covers it through the integer and object index + accessors, so the fix is pinned to the status rather than to one call site. + + Worth knowing for the next test written against this function: it returns + *the context it was given* when it does not handle the status, so + `TEST_EXPECT_OK` -- which releases whatever the statement returns -- will + double-release it against a `CLEANUP` block that also releases it, and a + double-released context corrupts the failure instead of reporting it. The + first draft of this test did exactly that and passed against the unfixed + library. It takes the result into a local and hands ownership over + explicitly now. 19. **The background music never loops.** `src/assets.c:20` initialises `bgmprops` to 0, `src/assets.c:44` sets `MIX_PROP_PLAY_LOOPS_NUMBER` on it, @@ -975,15 +982,16 @@ without coming here first. Ordered by blast radius. running valgrind. Recorded here only so the duplicate does not read as outstanding. -25. **`akgl_character_load_json_state_int_from_strings` checks the same argument - twice.** `src/character.c:123` guards `states` and `src/character.c:124` - guards `states` again under the message "NULL destination integer" — the - guard for `dest` was never written. A `NULL` `dest` is dereferenced at - `src/character.c:132`. Only reachable from inside the character loader, which - always passes a real pointer, so it is a latent hole rather than a live bug. +25. **`character_load_json_state_int_from_strings` checks the same argument + twice.** **Fixed in 0.5.0**: the second guard's subject is `dest`, which is + what it was always meant to be. - Fix: change the second guard's subject to `dest`. Touches - `src/character.c:124`. + **Not asserted, deliberately.** The function is `static` and has one call + site, which passes `&stateval` -- so a `NULL` `dest` is unreachable from the + public API and the guard cannot fire. Testing it would mean giving the + function external linkage purely to reach a defensive check, which undoes + internal-consistency item 9. The fix is a one-word correction to a guard + that is there for the next call site, not for this one. 26. **`akgl_actor_render` computes a sprite's drawn height from its width.** **Fixed in 0.5.0**: `dest.h` takes `curSprite->height`. Every actor was @@ -999,22 +1007,17 @@ without coming here first. Ordered by blast radius. ### Found while closing the akbasic API gaps 27. **`akgl_text_rendertextat` refuses the empty string, and `akgl_text_measure` accepts it.** - SDL_ttf returns `NULL` with "Text has zero width" from both `TTF_RenderText_Blended` and - `TTF_RenderText_Blended_Wrapped` for `""`, so `src/text.c:56`'s `FAIL_ZERO_RETURN` on the - surface reports `AKERR_NULLPOINTER` for what a caller means as "draw nothing". Verified - directly against SDL_ttf 3.x with the fixture font. + **Fixed in 0.5.0**: it returns success without rasterizing when + `text[0] == '\0'`, matching the measure side. - `akgl_text_measure("")` is documented as legal and returns 0 wide by one line high, which - is what a cursor sitting on an empty line needs, so the two halves of the same header - disagree about the same string. The functional consequence is a caller that draws a line of - text which may be empty — a line editor, a `PRINT` of an empty string, a HUD field that has - not been filled in yet — has to test for it before every call or handle a failure that - means nothing went wrong. + The check sits after the font, text and backend guards rather than before + them, so drawing nothing still refuses the things drawing something refuses + -- a caller does not get a different contract for an empty string. - **Fix:** return success without rasterizing when `text[0] == '\0'`, matching the measure - side, and say so in the header. Touches `akgl_text_rendertextat` only. `tests/text.c` has - the case written and deliberately not asserted; it would become a `TEST_EXPECT_OK`. Until - then the header carries the wart as a `@note` pointing here. + `tests/text.c` had the case written and deliberately unasserted, waiting for + the two halves of the header to agree. It is a `TEST_EXPECT_OK` now, on both + the wrapped and unwrapped paths, and against the old code it reports + `AKERR_NULLPOINTER`. ## Performance diff --git a/src/character.c b/src/character.c index 06ba1b1..fda9c8f 100644 --- a/src/character.c +++ b/src/character.c @@ -142,7 +142,7 @@ static akerr_ErrorContext *character_load_json_state_int_from_strings(json_t *st akgl_String *tmpstring = NULL; PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, states, AKERR_NULLPOINTER, "NULL states array"); - FAIL_ZERO_RETURN(errctx, states, AKERR_NULLPOINTER, "NULL destination integer"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination integer"); ATTEMPT { CATCH(errctx, akgl_heap_next_string(&tmpstring)); diff --git a/src/game.c b/src/game.c index 1347154..e30e08f 100644 --- a/src/game.c +++ b/src/game.c @@ -13,6 +13,7 @@ #include #include + #include #include #include @@ -50,13 +51,34 @@ 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 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. + */ +#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 at #AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH; - * the other three are name lengths half that or less. + * The spritesheet table is the widest; the other three are half that or less. */ -#define AKGL_GAME_SAVE_MAX_NAME_FIELD AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH +#define AKGL_GAME_SAVE_MAX_NAME_FIELD AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH /** * @brief Fail the build if a table ever declares a field wider than the staging buffer. @@ -66,10 +88,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_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]; + ((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]; /** * @brief Write a registry key as a fixed-width, zero-padded field. @@ -299,7 +321,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_ACTOR_MAX_NAME_LENGTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_ACTOR_NAME_WIDTH, fp)); actor = SDL_GetPointerProperty(props, name, NULL); CATCH(errctx, write_exact(&actor, 1, sizeof(akgl_Actor *), fp)); } CLEANUP { @@ -328,7 +350,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_SPRITE_MAX_NAME_LENGTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_SPRITE_NAME_WIDTH, fp)); CATCH(errctx, write_exact(&sprite, 1, sizeof(akgl_Sprite *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -360,7 +382,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_SPRITE_SHEET_MAX_FILENAME_LENGTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH, fp)); CATCH(errctx, write_exact(&spritesheet, 1, sizeof(akgl_SpriteSheet *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -388,7 +410,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_CHARACTER_MAX_NAME_LENGTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH, fp)); CATCH(errctx, write_exact(&character, 1, sizeof(akgl_Character *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -619,16 +641,34 @@ 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_ACTOR_MAX_NAME_LENGTH, sizeof(akgl_Actor *), AKGL_REGISTRY_ACTOR)); + CATCH(errctx, load_objectnamemap(fp, actormap, AKGL_GAME_SAVE_ACTOR_NAME_WIDTH, sizeof(akgl_Actor *), AKGL_REGISTRY_ACTOR)); // Load sprite name map spritemap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, spritemap, AKGL_ACTOR_MAX_NAME_LENGTH, sizeof(akgl_Sprite *), AKGL_REGISTRY_SPRITE)); + CATCH(errctx, load_objectnamemap(fp, spritemap, AKGL_GAME_SAVE_SPRITE_NAME_WIDTH, sizeof(akgl_Sprite *), AKGL_REGISTRY_SPRITE)); // Load spritesheet name map spritesheetmap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, spritesheetmap, AKGL_ACTOR_MAX_NAME_LENGTH, sizeof(akgl_SpriteSheet *), AKGL_REGISTRY_SPRITESHEET)); + CATCH(errctx, load_objectnamemap(fp, spritesheetmap, AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH, sizeof(akgl_SpriteSheet *), AKGL_REGISTRY_SPRITESHEET)); // Load character name map charactermap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, charactermap, AKGL_ACTOR_MAX_NAME_LENGTH, sizeof(akgl_Character *), AKGL_REGISTRY_CHARACTER)); + CATCH(errctx, load_objectnamemap(fp, charactermap, AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH, 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 + // a zeroed sentinel, so a reader whose field width disagrees with the + // writer's does not run off anything -- it finds a run of zeros + // somewhere inside an entry, stops early, and reports success with + // silently wrong maps. This is what makes that disagreement an error + // rather than a corruption, and it is the check that would have caught + // the spritesheet width being wrong for however long it was. + // + // It also has to move once the objects themselves are written; the + // comment below is the marker for that. + FAIL_NONZERO_BREAK( + errctx, + (fgetc(fp) != EOF), + AKERR_IO, + "Savegame has data left after its name tables; a name field width does not match the writer's"); + // Now that we have all of our pointer maps built, we can load the actual binary objects and reset their pointers } CLEANUP { if ( fp != NULL ) { diff --git a/src/json_helpers.c b/src/json_helpers.c index af47d18..ef9ef09 100644 --- a/src/json_helpers.c +++ b/src/json_helpers.c @@ -186,7 +186,17 @@ akerr_ErrorContext *akgl_get_json_with_default(akerr_ErrorContext *e, void *defv ATTEMPT { } CLEANUP { } PROCESS(e) { + // All three arms fall into the memcpy: HANDLE_GROUP emits no `break`, + // so a new status has to be added *above* the arm holding the body, not + // below it. + // + // AKERR_OUTOFBOUNDS is what akgl_get_json_array_index_object, _integer + // and _string report for a short array. Without it here, "this element + // is optional" worked for an object member and silently did not work + // for an array element -- the error propagated instead of being + // replaced by the default. } HANDLE_GROUP(e, AKERR_KEY) { + } HANDLE_GROUP(e, AKERR_OUTOFBOUNDS) { } HANDLE_GROUP(e, AKERR_INDEX) { memcpy(dest, defval, defsize); } FINISH(e, true); diff --git a/src/text.c b/src/text.c index 6a78d44..ebd0bfc 100644 --- a/src/text.c +++ b/src/text.c @@ -102,6 +102,15 @@ akerr_ErrorContext *akgl_text_rendertextat(TTF_Font *font, char *text, SDL_Color FAIL_ZERO_RETURN(errctx, akgl_renderer, AKERR_NULLPOINTER, "No renderer backend"); FAIL_ZERO_RETURN(errctx, akgl_renderer->sdl_renderer, AKERR_NULLPOINTER, "No valid SDL rendering backend"); FAIL_ZERO_RETURN(errctx, akgl_renderer->draw_texture, AKERR_NULLPOINTER, "Renderer backend has no draw_texture"); + // Drawing nothing is not a failure. SDL_ttf returns NULL with "Text has + // zero width" from both rasterizers for "", so this used to report + // AKERR_NULLPOINTER for what a caller means as "draw an empty line" -- + // while akgl_text_measure("") is documented as legal and returns 0 wide by + // one line high. Two halves of one header disagreeing about one string. + if ( text[0] == '\0' ) { + SUCCEED_RETURN(errctx); + } + if ( wraplength > 0 ) { textsurf = TTF_RenderText_Blended_Wrapped( font, diff --git a/tests/game.c b/tests/game.c index fed01ab..2c97a27 100644 --- a/tests/game.c +++ b/tests/game.c @@ -513,6 +513,77 @@ akerr_ErrorContext *test_game_updateFPS(void) SUCCEED_RETURN(e); } +/** + * @brief A save with a registered spritesheet must read back. + * + * The four name tables carry no length prefix, so the reader finds each entry + * by stepping a fixed width. 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, and that was enough: every entry after + * it was read out of the middle of its neighbour. + * + * The existing roundtrip test passed because empty registries write nothing but + * the zeroed sentinel. This one puts a name in each of the four registries, and + * a long one in the spritesheet registry, so the widths actually have to agree. + * + * The registry values are placeholder pointers rather than real objects: the + * save tables record name-to-address pairs and the loader looks each name up in + * the live registry, so what the pointers point at never matters here. + */ +akerr_ErrorContext *test_game_save_roundtrip_with_a_spritesheet(void) +{ + PREPARE_ERROR(e); + char longsheetname[AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH]; + akgl_Actor placeholder_actor; + akgl_Sprite placeholder_sprite; + akgl_SpriteSheet placeholder_sheet; + akgl_Character placeholder_character; + int i = 0; + + ATTEMPT { + CATCH(e, akgl_registry_init()); + CATCH(e, akgl_heap_init()); + set_game_identity(); + + // A spritesheet name that does not fit the width the reader used to + // assume. Filled to just under the field so the terminator still fits. + memset(&longsheetname, 0x00, sizeof(longsheetname)); + for ( i = 0; i < (AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH - 1); i++ ) { + longsheetname[i] = 'a' + (i % 26); + } + + FAIL_ZERO_BREAK(e, SDL_SetPointerProperty(AKGL_REGISTRY_ACTOR, "roundtrip_actor", + (void *)&placeholder_actor), + AKERR_KEY, "could not register the actor"); + FAIL_ZERO_BREAK(e, SDL_SetPointerProperty(AKGL_REGISTRY_SPRITE, "roundtrip_sprite", + (void *)&placeholder_sprite), + AKERR_KEY, "could not register the sprite"); + FAIL_ZERO_BREAK(e, SDL_SetPointerProperty(AKGL_REGISTRY_SPRITESHEET, (char *)&longsheetname, + (void *)&placeholder_sheet), + AKERR_KEY, "could not register the spritesheet"); + FAIL_ZERO_BREAK(e, SDL_SetPointerProperty(AKGL_REGISTRY_CHARACTER, "roundtrip_character", + (void *)&placeholder_character), + AKERR_KEY, "could not register the character"); + + TEST_EXPECT_OK(e, akgl_game_save((char *)&savepath), + "saving a game with all four registries populated"); + + // The load is what walks the four tables in order. If any width + // disagrees with the writer's, the table after it starts mid-entry and + // the read runs off the end of the file. + TEST_EXPECT_OK(e, akgl_game_load((char *)&savepath), + "loading back a save with a registered spritesheet"); + + TEST_ASSERT(e, strncmp((char *)&akgl_game.name, "libakgl test game", 256) == 0, + "the game identity did not survive the roundtrip"); + } CLEANUP { + unlink((char *)&savepath); + } PROCESS(e) { + } FINISH(e, true); + SUCCEED_RETURN(e); +} + /** @brief Counts akgl_game_update calls into each actor's updatefunc, by actor index. */ static int updatecounts[AKGL_MAX_HEAP_ACTOR]; @@ -669,6 +740,7 @@ int main(void) CATCH(errctx, test_game_save_load_nullpointers()); CATCH(errctx, test_game_load_truncated_table()); CATCH(errctx, test_game_save_writes_name_tables()); + CATCH(errctx, test_game_save_roundtrip_with_a_spritesheet()); CATCH(errctx, test_game_state_lock()); CATCH(errctx, test_game_state_lock_budget()); CATCH(errctx, test_game_updateFPS()); diff --git a/tests/json_helpers.c b/tests/json_helpers.c index 9f35a4b..ffd088b 100644 --- a/tests/json_helpers.c +++ b/tests/json_helpers.c @@ -418,6 +418,9 @@ akerr_ErrorContext *test_json_with_default(void) int defval = 99; akerr_ErrorContext *keyerr = NULL; akerr_ErrorContext *typeerr = NULL; + akerr_ErrorContext *defaulted = NULL; + json_t *shortarray = NULL; + json_t *junkobj = NULL; int junk = 0; ATTEMPT { @@ -446,6 +449,51 @@ akerr_ErrorContext *test_json_with_default(void) TEST_ASSERT(e, dest == 1, "with_default applied the default for an unrelated error"); typeerr = NULL; + // A short array. This is the one that did not work: the three array + // index accessors report AKERR_OUTOFBOUNDS, which with_default never + // handled, so "this element is optional" worked for an object member + // and silently did not for an array element. Nothing in tree passed an + // array accessor's error here, which is why it went unnoticed. + dest = 1; + CATCH(e, akgl_get_json_array_value(fixture, "integers", &shortarray)); + keyerr = akgl_get_json_array_index_integer(shortarray, 99, &junk); + TEST_ASSERT(e, keyerr != NULL, "reading past the end of an array unexpectedly succeeded"); + TEST_ASSERT(e, keyerr->status == AKERR_OUTOFBOUNDS, + "a short array reported %d, expected AKERR_OUTOFBOUNDS", keyerr->status); + // Not TEST_EXPECT_OK here. That macro releases whatever context the + // statement returns, and akgl_get_json_with_default returns *the context + // it was given* when it does not handle it -- so on the failing path the + // CLEANUP block below would release the same context a second time, and + // a double-released context corrupts the failure rather than reporting + // it. Ownership passes to with_default either way: it releases through + // FINISH when it handles the status, and hands the context back when it + // does not. + defaulted = akgl_get_json_with_default(keyerr, (void *)&defval, (void *)&dest, sizeof(int)); + keyerr = NULL; + if ( defaulted != NULL ) { + defaulted->handled = true; + defaulted = akerr_release_error(defaulted); + FAIL_BREAK(e, AKGL_ERR_BEHAVIOR, + "with_default propagated AKERR_OUTOFBOUNDS instead of applying the default"); + } + TEST_ASSERT(e, dest == 99, + "with_default did not apply the default for a short array (dest is %d)", dest); + + // The same through the object index accessor, so the fix is pinned to + // the status rather than to one call site. + dest = 1; + keyerr = akgl_get_json_array_index_object(shortarray, 99, &junkobj); + TEST_ASSERT(e, keyerr != NULL, "reading an object past the end unexpectedly succeeded"); + defaulted = akgl_get_json_with_default(keyerr, (void *)&defval, (void *)&dest, sizeof(int)); + keyerr = NULL; + if ( defaulted != NULL ) { + defaulted->handled = true; + defaulted = akerr_release_error(defaulted); + FAIL_BREAK(e, AKGL_ERR_BEHAVIOR, + "with_default propagated a short object array instead of applying the default"); + } + TEST_ASSERT(e, dest == 99, "with_default did not apply the default (dest is %d)", dest); + // NULL arguments alongside a real error are a contract violation. keyerr = akgl_get_json_integer_value(fixture, "absent", &junk); TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, diff --git a/tests/text.c b/tests/text.c index b9abf08..2771ef0 100644 --- a/tests/text.c +++ b/tests/text.c @@ -310,10 +310,25 @@ akerr_ErrorContext *test_text_rendertextat(void) TEST_EXPECT_OK(errctx, akgl_text_rendertextat(testfont, "one two three", white, TEST_TARGET_SIZE / 2, 0, 0), "drawing wrapped text"); - // Not asserted here: the empty string, which SDL_ttf refuses with "Text - // has zero width" on both paths, so drawing an empty line reports a - // failure rather than drawing nothing. That is filed rather than pinned - // -- see TODO.md, "Known and still open". + // The empty string. SDL_ttf refuses it with "Text has zero width" from + // both rasterizers, so this used to report AKERR_NULLPOINTER for what a + // caller means as "draw nothing" -- while akgl_text_measure("") is + // documented as legal. This case was written and deliberately left + // unasserted until the two halves of the header agreed. + TEST_EXPECT_OK(errctx, akgl_text_rendertextat(testfont, "", white, 0, 0, 0), + "drawing an empty line of text"); + TEST_EXPECT_OK(errctx, akgl_text_rendertextat(testfont, "", white, TEST_TARGET_SIZE / 2, 0, 0), + "drawing an empty line of wrapped text"); + + // Drawing nothing must still refuse the things drawing something + // refuses, rather than short-circuiting past the checks. + TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, + akgl_text_rendertextat(NULL, "", white, 0, 0, 0), + "drawing an empty line with no font"); + TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, + akgl_text_rendertextat(testfont, NULL, white, 0, 0, 0), + "drawing a NULL string"); + TEST_EXPECT_OK(errctx, akgl_renderer->frame_end(akgl_renderer), "ending the frame"); } CLEANUP { } PROCESS(errctx) {