Five follow-ups from earlier review feedback - #575
Draft
AdamCoulterOz wants to merge 5 commits into
Draft
Conversation
can_read_n rejects a negative count itself, and says so in its own comment, so testing for one here repeats the guard rather than adding to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both make_chunk helpers shifted the length field into place a byte at a time. bstone_endian.cpp is already in this target, so write_u16_le and write_u32_le say the same thing and are the codebase's answer for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Choosing a library was split between get_oal_default_library_file_name and a second policy inlined in initialize_oal, which loaded each candidate and swallowed the failure to find out whether it was there. Selection now lives in get_oal_preferred_library_file_name beside the default one, and asks sys::is_regular_file_exists the way the rest of the codebase does, leaving initialize_oal with a single load. A candidate that exists but cannot be loaded now reports that instead of quietly falling back to the deprecated framework. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every other R3r validator is a static member of R3rUtils; this one was a free function in bstone_r3r.h. Testing it costs the test target eight translation units, because R3rUtils reaches the sprite, window and cvar code. That is the price of putting the validator where the others live, rather than defining it in the header to dodge the link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing includes bstone_sys_window_size.h and no CMakeLists lists it, and it could not compile if it were used: it includes bstone_sys_r2_extent.h, which does not exist, and derives from R2Extent, which is not a type. It also declares a bstone::sys::WindowSize incompatible with the live one in bstone_sys_window.h. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AdamCoulterOz
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five spots where the patterns from your earlier review comments applied but hadn't been picked up yet. Held back while the previous batch was still open so they wouldn't collide; they're independent of each other, so any of them can be dropped without disturbing the rest.
Two of them follow your answers in #574 — thanks for those.
[Sys]Delete the unreferenced window-size headerPer your "it is safe to delete the file". Nothing includes
bstone_sys_window_size.h, no CMakeLists lists it, and it could not compile if it were used: it includesbstone_sys_r2_extent.h, which does not exist, and derives fromR2Extent, which is not a type. It also declares abstone::sys::WindowSizeincompatible with the live one inbstone_sys_window.h.Closes #574.
[R3r]Home the read-pixels validator with the other validatorsr3r_validate_read_pixels_paramwas a free function inbstone_r3r.h, while every other R3r validator is a static member ofR3rUtils. This is yourread_u32_becomment on #563 in a different shape, so it's nowR3rUtils::validate_read_pixels_param, declaration and definition split as you asked.The cost, for the record: its body lives in
bstone_r3r_utils.cppwith its siblings, so the unit tests need that translation unit, and it brings seven more with it —bstone_cvar,bstone_cvalidator,bstone_cvar_string,bstone_rgb8,bstone_sprite,bstone_sys_window,bstone_video_cvars— becauseR3rUtilsalso reaches the sprite, window and cvar code.[OpenAL]Give the library preference one home and a proper probeLibrary selection was split between
get_oal_default_library_file_name()and a second policy inlined ininitialize_oal(), which discovered whether a candidate was present by loading it and swallowing the failure. Selection now lives inget_oal_preferred_library_file_name()beside the default one, askssys::is_regular_file_existsthe way the rest of the codebase does, and leavesinitialize_oal()with a single load.One behaviour change: a candidate that exists but cannot be loaded now reports that, instead of quietly falling back to the deprecated framework.
[Tests]Store the OPL chunk headers with the endian helpersBoth
make_chunkhelpers shifted the length field into place a byte at a time.bstone_endian.cppis already in that target, sowrite_u16_leandwrite_u32_lesay the same thing.[Movie]Drop the negative checkcan_read_nalready makesSince
can_read_nrejects a negative count itself — and says so in its own comment — testing for one at the call site repeats the guard rather than adding to it.Built clean and the full suite passes (514 tests, 0 failures), including the 6 covering the moved validator and the 14 covering the two OPL decoders.
🤖 Generated with Claude Code