Skip to content

Fix vkfw module (and clean up)#26

Closed
Temaa123 wants to merge 1 commit into
Cvelth:mainfrom
Temaa123:main
Closed

Fix vkfw module (and clean up)#26
Temaa123 wants to merge 1 commit into
Cvelth:mainfrom
Temaa123:main

Conversation

@Temaa123
Copy link
Copy Markdown

Changes

  1. vkfw.cppm no longer includes vkfw.hpp. This means the code is duplicated, but I think this is the way forward, since it both fixes a lot of errors and warnings, and also this bumps up the minimum C++ standard for vkfw.cppm to C++20, which could allow a lot of cleanup.
  2. I tried not to touch things that could theoretically help someone, but I did remove a lot of unnecessary C++ standard version checks, and checks for if this is the module.
  3. I moved the check for GLFW version / target version mismatch outside the module fragment, which removes a warning.

Checked on MSVC. I'd be grateful if someone checks on GCC/Clang, but I don't see a way this could go wrong anyway.

@Cvelth
Copy link
Copy Markdown
Owner

Cvelth commented May 28, 2026

  1. What kind of errors/warnings are talking about? I'd really prefer to avoid code duplication if possible.

2-3. Even more so if the two files are not identical. So it's not even possible to use basic diff to make sure two stay in sync.

@Temaa123
Copy link
Copy Markdown
Author

Well, the error is exporting modules inside of an #include. The first warning is exporting modules inside of an #ifdef statement(#ifdef VKFW_MODULE_IMPLEMENTATION). The second warning is that only preprocessor directives can appear in a global module fragment, but there is a static_assert to check that the GLFW version is correct.

Also I think you could realistically drop header support, since this library is the more "modern" approach to Vulkan anyway, and C++20 did come out ~6 years ago and many people say that C++20 is a good baseline for gamedev nowadays.

You could also remove a LOT of macros like VKFW_CONSTEXPR, which could improve code readability, and maybe even compile times.

But the choice is up to you, I'm probably missing something anyway.

@Cvelth
Copy link
Copy Markdown
Owner

Cvelth commented May 29, 2026

Well, the error is exporting modules inside of an #include.

I faintly remember talking about something like that in the past. For some context, my only interaction with the modules was a while ago. And they were still severely underbaked.

I wonder if this is a matter of hoisting just the export module X line out of the header 🤔

The first warning is exporting modules inside of an #ifdef statement(#ifdef VKFW_MODULE_IMPLEMENTATION).

I don't think there's much we can do about this one. Luckily it's just a warning.

The second warning is that only preprocessor directives can appear in a global module fragment, but there is a static_assert to check that the GLFW version is correct.

This one, on the other hand, sound easy to fix: we can just move it down. It's not like its exact position matters much. If it triggers, it triggers.

Also I think you could realistically drop header support, since this library is the more "modern" approach to Vulkan anyway, and C++20 did come out ~6 years ago and many people say that C++20 is a good baseline for gamedev nowadays.

Which means we are cutting out all projects started before it became a thing. Considering that libraries like this are more likely to be used for hobby projects, I'd imagine it would break quite a few - not everyone has time to be constantly updating theirs. If anything, I'd avoid breaking any existing code unless we have a really strong reason to!

Also, while I agree that C++20 is a decent baseline for new projects, modules are definitely not there yet:

https://arewemodulesyet.org

You could also remove a LOT of macros like VKFW_CONSTEXPR, which could improve code readability, and maybe even compile times.

Yes. If I were to start writing this thing today, I'd not use a macro for that. And yes, I agree it's ugly, but I can't remember a single time it caused any trouble. 'Let sleeping dogs lie,' as they say. IMO, it's worth keeping, even if it's just for that one (hopefully not imaginary) person who still uses C++11.


To summarize, here's my general stance:

  1. Let's not break any existing code using this library, unless we gain considerable benefit from doing so. And those benefits cannot be obtained any other way.

  2. Let's try to limit code duplication. In my personal experience, when there are two copies of a thing, sooner or later they go out of sync. This doesn't get many PRs, and I'd hate to discourage future contributors even more by asking them to do (and most importantly test) the work twice. Second time for a use-case they probably don't even care about.

  3. With those two in mind, I'm open to supporting as many possible new use-cases as I can. I don't have much time to dedicate to this, but I'm always willing to merge new stuff in. As long as it doesn't actively degrade the experience for anyone else.


So, that out of the way, are you willing to try and massage the PR in a way that would allow us to achieve what you want without doubling the codebase size? Or, otherwise, prove that it cannot be done (then we'll have to think about the least dangerous way to duplicate the sources)?

If you feel like that's too much of an undertaking, that's fine. I'm willing to provide whatever help with it I can give (as long as it doesn't cost me too much time :D). Be it answering any questions you might have or investigating any blockers you run into.

@Cvelth
Copy link
Copy Markdown
Owner

Cvelth commented May 29, 2026

Specifically, I'm thinking about something like this:
da8842e (from #21)

Can you try something like that first? It probably needs more massaging, but hopefully it does the trick. I didn't merge it then because I didn't have an easy way of testing modules yet (gcc-16 it was targeting still needed to be built from source, if I recall correctly). But, if it works for you, maybe I can try again. Surely the situation is better now that it was a year ago when I wrote that.

@Temaa123 Temaa123 closed this May 31, 2026
@Temaa123
Copy link
Copy Markdown
Author

Closing this, and I will make a new pull request when I'm done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants