-
Notifications
You must be signed in to change notification settings - Fork 342
Use the ABI-breaking style for the C++ module #2443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Use the ABI-breaking style for the C++ module #2443
Conversation
a7c5fbb to
3527223
Compare
M2-TE
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply VULKAN_HPP_EXPORTing the entire namespace is a good solution!
The real test would be to run the Handles and ArrayWrapper module tests (once you have the vulkan module changes), as those currently don't work due to lack of exported operators.
|
I've actually got to the next step and passed all module tests locally, but it's a bit of a mess because We would have to drop the partition altogether, directly include Now, this is OK when manually editing the files (I should commit and push what I have, coming shortly...), but generating the headers is a problem, especially when it comes to the |
The tests I mentioned before are currently disabled, you have to manually uncomment them in
I don't think that would be too much of a problem. The video module is kind of unnecessary when all the export statements are in the hpp headers anyways. |
That's not the biggest issue, but it's generating video-related stuff in the In fact it might be better for |
- separating it out again - `import vulkan`
|
OK, this latest set of patches implements that; we are back to two modules: IMO this is the most logical separation, as it is clear the video module depends on Vulkan-Hpp core functionality, but the normal Vulkan module doesn't need any video-related functionality. As a stretch goal I would also like to get rid of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adopts the "ABI-breaking style" for C++ modules as documented in the Clang migration guide, simplifying the module interface generation by using a VULKAN_HPP_EXPORT macro instead of explicit using statements. This resolves issues with maintaining hardcoded using statements and simplifies the code generator.
Changes:
- Introduced
VULKAN_HPP_EXPORTmacro that expands toexportin module context - Simplified
vulkan_video.cppmby replacing 300+ lines ofusingstatements with direct namespace exports - Updated all header files to use
VULKAN_HPP_EXPORTon namespace declarations
Reviewed changes
Copilot reviewed 30 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vulkan/vulkan_video.cppm | Simplified from ~320 lines to ~72 lines by removing explicit using statements, now imports vulkan module and includes headers with export |
| vulkan/vulkan_video.hpp | Wrapped includes in !defined(VULKAN_HPP_CXX_MODULE) guard, added VULKAN_HPP_EXPORT to namespace, formatting improvements |
| vulkan/vulkan_hpp_macros.hpp | Added VULKAN_HPP_EXPORT macro definition |
| snippets/*.hpp | Updated templates to apply VULKAN_HPP_EXPORT pattern |
| VulkanHppGenerator.cpp | Removed generation of using statements for modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Currently, the module breaks for me (Clang 21) when using the dynamic dispatcher. The Simply moving Could we use this opportunity to think about removing the need to have external storage for the dynamic dispatcher when using the module? It makes sense for the normal headers, but the module is its own TU. |
Yes; if the module is being used we can probably opt into having the storage directly in the module. I've been thinking about the |
|
You forgot to export the namespace with
Agreed, users often interact with it directly (initialization and storage in particular, sometimes even passing it to functions). Easiest way I can get the dynamic dispatcher compiled within the module is by:
Of course, these should be hidden by similar |
|
I don't really use the dynamic dispatcher (I predominantly use |
|
The order declarations/definitions is a bit odd. As far as I can tell, it is a sort of circular dependency? As an example if I do not forward declare I can create a patch off of your branch with a working implementation of what I mentioned, if you want? |
By all means; I can accept that in when you think it's ready. Cheers! |
|
The patch: dyndis.patch What this does is three things:
These changes honestly belong into their own PR, as they touch on the normal headers as well. I don't want such a change to drown in an already big PR like this. Especially since it should also work with the current main branch modules (just gotta add the export keyword). What do you think? |
|
I reckon if we want to separate it out, we should merge that in first, and then I can rebase my work on top of that. Although I suspect the rebase will be quite painful, and it might be easier to just add this patch in, it's not the biggest, really. I absolutely want to avoid a 1.4.335 situation where we had a broken master branch, and it seems this might be such a case, because we haven't enabled those tests. |
- Move forward declarations to `vulkan.hpp` - Declare storage in the module - Fix tests
|
Side question... Could we make the module tests reuse the compiled module? As they are now, the test timings don't look very flattering because the module is recompiled for every test. |
|
I had separated them via ctest, because some tests need special Previously I had made a sort of "hash map" in the root Vulkan-Hpp CMake, where tests with the same flags would share the same Vulkan module dependency.. but it was incredibly messy and error prone (leaking flags, etc..). Perhaps this is worth revisiting.. |
- Also enable tests
|
Fair enough. Although I'd really like to prepare some header-vs-module timings. Hopefully I can migrate the modern Khronos samples to this repository... some day. |
|
The tests would be a bad fit for timings either way. You would need an atleast somewhat larger project (like those samples) where multiple TUs otherwise include By the way, the current state of this PR is now fully working for me (Clang 21). |
Woohoo! Works for me on MSVC too. @asuessenbach this is ready to run CI and for your review... |
|
I sadly don't have much experience with modules and rarely use them. Tried to get this work on our version of the Vulkan Tutorial with MSVC 2022, but I'm stuck with the following error: "vulkan.cppm(8,1): error C7577: a global module fragment can only appear at the start of a translation unit" I have enabled both the ISO C++23 langauge preview and Build ISO C++23 Standard Library modules |
|
@SaschaWillems that sounds like you have precompiled headers enabled. I think I had that issue for that reason before, since it places the pch at the beginning where the global module fragment is supposed to be. |
|
Just checked, precompiled headers are explicitly disable (option is set to "Not Using Precompiled Headers"). |
|
Hm.. something is either being placed at the start of the module automatically, or you perhaps included |
Resolves #2366 and resolves #2379.
Also resolves #2447 and resolves #2448.
Starts with an initial experiment with
vulkan:video, which seems to work pretty well with theMODULE_Videotest.The only issue is the duplicated
#includes in both the.hppand the.module, but I would rather duplicated headers than all the issues with theusingstatements. The module interface file is also so much shorter now.