fix: windows compile with ninja/clang

[vikhik]

Pull Request Template

Description

Building and compiling on Windows with Ninja + Clang doesn't work, so I fixed it.

Type of change

Please delete options that are not relevant.

• [x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• cmake -B build . -G "Ninja Multi-Config"
• cmake --build build -C Release

Test Configuration:

• Windows 10
• cmake version 3.31.6
• ninja version 1.21.1
• clang version 21.1.7
• Visual Studio 2022 installed for msvcrt

[vikhik]

Latest changes now cause clang to compile with zero warnings on Windows

[robertosfield]

Do you have instructions or github actions to test the Cygwin build? I can verify that it should not break.

This action is a community contribution, needs to be invoked manually as for now it's not run autoamtically on check-ins: https://github.com/vsg-dev/VulkanSceneGraph/actions/workflows/win-cygwin-mingw.yml

[vikhik]

Do you have instructions or github actions to test the Cygwin build? I can verify that it should not break.

This action is a community contribution, needs to be invoked manually as for now it's not run autoamtically on check-ins: https://github.com/vsg-dev/VulkanSceneGraph/actions/workflows/win-cygwin-mingw.yml

I have run the workflow here: https://github.com/vikhik/VulkanSceneGraph/actions/runs/20207232936

cygwin-shared passes, but the other jobs fail based on unrelated things (old Actions, incorrect Vulkan setup, etc.).

[AnyOldName3]

At the minimum, the description of this PR is wrong - I've got Ninja working locally with MSVC natively, and GCC and Clang via MSYS2. While we know there are problems with clang-cl, from looking at the changes you've made, it seems like the problems you're hitting are entirely unrelated to Ninja (otherwise you'd have had to change the build system, i.e. the CMake, rather than the C++), and are probably specific to Clang via Cygwin (otherwise I'd have run into them via MSYS2).

[AnyOldName3]

Actually, looking a second time, most of these changes aren't build fixes at all, they're just warning fixes, which would only cause a build failure if you'd manually opted into -Werror, which isn't enabled by default. Some of these warnings do fire for me, and aren't necessarily Windows-specific (e.g. the rule-of-three violation in MemoryBlock::Element causes a warning anywhere with a new enough Clang version) or Clang-specific (e.g. the Win32_Window.h unused variable causes a warning in GCC, too). They're basically happening because the VSG has warnings disabled for MSVC, and I've not got around to fixing warnings in other compilers that Robert doesn't use.

[robertosfield]

I think the best thing to do is separate out warning fixes into batches.

Or better yet post the warnings so I can review and choose the most appropriate way to fix them. One has to be careful just quietening warnings as one can introduce bugs and lower code readability.

For instance [[maybe_unused]] use is hacky, it stops a warning that is better fixed by reviewing the wider code to see if the variable should be used but an oversight later in the code means it's not and that's a bug. With this usage the reader is left wondering what is intended. This all gets in the way of developers reading the code spotting the relevant bits of code that affect the behavior of the code, this might not seem like a big deal, but when you have hundreds of lines of code that you are reviewing the harder it is to glean the real intended purpose and actual implementation behavior the more likely bugs will be introduced or not spotted.

So each time we change the code we want to move in the direction of efficiency, readability and maintainability. Code quality over decades of development is like fighting entropy, the code will tend to chaos unless you keep resetting it back on the right path.

[AnyOldName3]

With the unused variable, the context is that we're explicitly converting the scancode to a virtual key via MapVirtualKeyEx, so ignoring the virtual keycode from the function parameter. https://github.com/vsg-dev/VulkanSceneGraph/pull/1248 was the PR that most recently modified the behaviour. Before it, there was not-quite-accurate conversion of the scancode, and it improved the conversion, but none of the comments say whether it's ignoring the virtual keycode we're given in favour of computing it just to copy what the function did before the PR or whether there's some problem with the keycode we're given.

[vikhik]

That's fair feedback. The main change that is required to use Ninja and clang together (not ninja+msvc, etc.) is the macro change in the file system. If you like, I'll reset the PR to just do that which is required for compilation and not also the warning suppression.

[AnyOldName3]

Again, which environment's Clang? MSYS2 Clang works fine for me (beyond there being some extra warnings because it's a newer version than is used for Android or MacOS in CI).

[vikhik]

Standard clang, not -cl, installed with a plain winget install llvm. Not using msys2 nor cygwin/etc., so it's using Microsoft's C++ standard library, but compiled with no extra flags etc.

[AnyOldName3]

I'm not sure that's standard at all - Clang on Windows is usually used via the clang-cl frontend or in an MSYS2/Cygwin environment unless you're compiling a web browser.

Anyway, it's a valid environment, so perfectly reasonable for the VSG to support it. I'm more confident that the check should be changed from WIN32 to _WIN32 instead of _WIN32 being added as an extra check now I know the situation you're trying to fix, and I also think that the other un-underscore-prefixed instances should be changed, too. If the one in executableFilePath isn't fixed, even though it'll build, that function won't work. The one in Export.h is also not vital for the build to work, but if it's not changed, the library will end up much larger than it needs to be, and you won't know if you've broken the other Windows builds by leaving off a VSG_DECLSPEC if you submit more patches later. (Also, there are reasons why we might want to make Export.h support the equivalent GCC/Clang attributes and not export everything by default on Unix, too, so the check might get removed altogether at some point.)

[vikhik]

I think it's becoming a more commonly supported workflow with e.g. VSCode/clangd integration, so it's probably good to support.

Would you like me to resubmit with those fixes you requested?

[vikhik]

I've updated the PR to be only about the _WIN32 macro, and can submit the other changes separately, later.