bstone_sys_window_size.h is unreferenced and would not compile

[AdamCoulterOz]

src/bstone/src/bstone_sys_window_size.h looks like a leftover. On wip it is not included by anything, is not listed in src/bstone/CMakeLists.txt, and could not compile if it were used:

#include "bstone_sys_r2_extent.h"   // no such file; the tree has bstone_r2_extent.h

namespace bstone {
namespace sys {

struct WindowSize : R2Extent      // no such type; bstone_r2_extent.h declares
{                                 // R2ExtentT<T> and R2ExtentI, in namespace bstone
    using R2Extent::R2Extent;
};

It also declares bstone::sys::WindowSize, which already exists as a different type — bstone_sys_window.h:109 defines it as a plain { int width; int height; }. Nothing includes both today, so the collision is dormant, but any file that ever did would not build.

I ran into it while looking at whether sys::RendererOutputSize in bstone_sys_renderer.h could reuse sys::WindowSize instead. It cannot as things stand — bstone_sys_window.h includes bstone_sys_renderer.h, so the renderer header cannot include the window one, and splitting the type into its own header is the natural fix. That is presumably what this file was for.

So either it is a half-finished split worth completing — in which case the include and the base type need correcting, and bstone_sys_window.h should use it rather than declare its own — or it is dead and can go. Happy to send a patch for whichever you prefer.

[AdamCoulterOz]

@bibendovsky — following up on this one. No urgency, but I'd rather ask than guess.

Which way would you like it resolved: complete the split, or delete the file?

I'm asking rather than just sending a patch because the two answers produce quite different diffs, and one of them changes bstone_sys_window.h — which seemed like the sort of thing you'd want a say in before it turns up as a PR.

Happy to do the whole job either way:

• Complete the split — correct the include to bstone_r2_extent.h and the base to R2ExtentI, have bstone_sys_window.h use bstone_sys_window_size.h rather than declaring its own WindowSize, and then make sys::RendererOutputSize in bstone_sys_renderer.h reuse it, which is what removes the duplicate that led me here.
• Delete it — drop the file and leave RendererOutputSize as it is.

Either way I'll send it as a tested PR, so there's nothing to do at your end beyond saying which you'd prefer.

---

Separately, and worth flagging here because it came out of your read_u32_be comment on #563: I've taken r3r_validate_read_pixels_param out of bstone_r3r.h and made it R3rUtils::validate_read_pixels_param, so it sits alongside validate_initialize_param and the others instead of being the one validator that lives as a free function.

The body goes in bstone_r3r_utils.cpp with its siblings, and that does cost something worth naming up front: the unit tests for it now need bstone_r3r_utils.cpp in the test target, and that pulls seven more translation units in with it — bstone_cvar, bstone_cvalidator, bstone_cvar_string, bstone_rgb8, bstone_sprite, bstone_sys_window, bstone_video_cvars — because R3rUtils also reaches the sprite, window and cvar code.

I did try defining it in the header to avoid that, and decided against it: it dodges the link cost by making one validator inconsistent with the rest, which is the thing the move was meant to fix. If you'd rather the test target not carry the extra translation units, I'm happy to do it another way — just let me know.

[bibendovsky]

Which way would you like it resolved: complete the split, or delete the file?

It is safe to delete the file.

The body goes in bstone_r3r_utils.cpp with its siblings, and that does cost something worth naming up front: the unit tests for it now need bstone_r3r_utils.cpp in the test target, and that pulls seven more translation units in with it — bstone_cvar, bstone_cvalidator, bstone_cvar_string, bstone_rgb8, bstone_sprite, bstone_sys_window, bstone_video_cvars — because R3rUtils also reaches the sprite, window and cvar code.

For now lets keep this function in class R3rUtils even if it will cost a little.
And you may split the method into declaration and definition.