Skip to content
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

Add C++20 module interface file and tests #1582

Merged
merged 30 commits into from
Jun 28, 2023

Conversation

sharadhr
Copy link
Contributor

@sharadhr sharadhr commented May 28, 2023

Resolves #1580.

VulkanHppGenerator and CMakeLists.txt have been modified to produce a C++20 vulkan.cppm module file that exports all types, structs, functions, names, handles, etc. used by Vulkan in the module, vulkan. C++20 users can now consume all of Vulkan-Hpp with a single line: import vulkan;.

Additional new features include consteval auto functions for macros that take numeric parameters, and constexpr auto variables for macros, both prefaced with c. So, VK_MAKE_API_VERSION is now vk::makeApiVersion, and is consteval-callable. A notable exclusion is VK_DEFINE_HANDLE, which performs macro magic, and cannot really be consteval-ed, trivially.


This is still a draft, as MSVC emits errors and warnings pertaining to vk::getDispatchLoaderStatic() being declared but not defined, and vk::anonymous-namespace::throwResultException() being, well, in an anonymous namespace. Resolved with 32d5a69 and c416dc2.

This is an RFC and I'm looking for feedback on my code. There are still a few things to be done, besides correcting the above-mentioned warnings and errors:

  • Copy and convert all of the existing suite of samples and tests to using vulkan;;
  • As mentioned in the TODO, some of the free functions in vulkan_funcs.hpp were eye-balled and hard-coded into the generator. It is very difficult to follow the recursive generateCommand... functions, and I'm hoping the maintainers have some input for this... Resolved with 7b4969b.
  • Somehow verify that all types currently available in vulkan.hpp and vulkan_raii.hpp indeed were exported, and they are correctly guarded by macros;
  • Export types from vulkan_format_traits.hpp, vulkan_extension_inspection.hpp and vulkan_hash.hpp. Resolved with 09a1177.

Thanks, and all feedback is welcome.

@CLAassistant
Copy link

CLAassistant commented May 28, 2023

CLA assistant check
All committers have signed the CLA.

@sharadhr sharadhr changed the title Add Vulkan C++20 module interface file and tests: #1580 Add Vulkan C++20 module interface file and tests: resolves #1580 May 28, 2023
@sharadhr
Copy link
Contributor Author

sharadhr commented May 30, 2023

With respect to c416dc2, the tests that make reference to it, specifically ArrayProxy.cpp, DesignatedInitializers.cpp, and StridedArrayProxy.cpp do not build successfully, even on the main branch.

I am using MSVC 19.35, Visual Studio 2022, CMake 3.26, Ninja 1.11.0. Not sure if they are broken tests or I'm doing something wrong.

vulkan/vulkan.ixx Outdated Show resolved Hide resolved
VulkanHppGenerator.hpp Outdated Show resolved Hide resolved
vulkan/vulkan.ixx Outdated Show resolved Hide resolved
@sharadhr
Copy link
Contributor Author

I was also thinking if we should add signpost comments to vulkan.ixx similar to the other header files, such as:

//===============
//=== STRUCTS ===
//===============

so that users know where each type is defined. Of course, IDEs come with a go-to-definition/declaration feature, but maybe this is useful. Any thoughts?

@Agrael1
Copy link
Contributor

Agrael1 commented Jun 5, 2023

Experimenting with exporting vulkan module I came to a point, where compiler started to complain about std::weak_ordering and other structs, I had to resort to VULKAN_HPP_NO_SPACESHIP_OPERATOR definition. Is that solved somehow?

@sharadhr
Copy link
Contributor Author

sharadhr commented Jun 5, 2023

@Agrael1, I have written a simple test, where I simply re-defined the vk::ApplicationInfo struct as applicationInfo2 in
tests/Cpp20Modules/Cpp20Modules.cpp, explicitly called operator<=>, with control flow depending on the result.

While Visual Studio's IntelliSense doesn't detect the VULKAN_HPP_HAS_SPACESHIP_OPERATOR macro for code highlighting, MSVC cl.exe 19.35 compiles the code just fine, and runs fine, too; the debugger correctly jumps into the correct operator<=>() defined for vk::ApplicationInfo and returns the correct value.

Cpp20Modules.zip

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, what a big PR!
And an important one as well. I'm pretty sure, having vulkan.hpp as a module will be very much appreciated by everyone who suffers from the long compile times when including it.
So, thanks a lot for your contribution here!
Besides all my comments below: maybe you could switch to a different clang-format version, to prevent all the formatting differences in vulkan.hpp and vulkan_raii.hpp. I think, 14.x is a good choice.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
VulkanHppGenerator.cpp Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
vulkan/vulkan.ixx Outdated Show resolved Hide resolved
vulkan/vulkan.ixx Outdated Show resolved Hide resolved
vulkan/vulkan.ixx Outdated Show resolved Hide resolved
vulkan/vulkan.ixx Outdated Show resolved Hide resolved
vulkan/vulkan.ixx Outdated Show resolved Hide resolved
@sharadhr sharadhr force-pushed the cpp20-modules branch 3 times, most recently from f50b70e to 2514ab0 Compare June 15, 2023 08:28
vulkan/vulkan.hpp Outdated Show resolved Hide resolved
vulkan/vulkan.hpp Outdated Show resolved Hide resolved
@sharadhr sharadhr force-pushed the cpp20-modules branch 3 times, most recently from 9e456b9 to 378087f Compare June 16, 2023 14:28
@sharadhr
Copy link
Contributor Author

sharadhr commented Jun 16, 2023

Okay; at this juncture, I think the PR is ready for complete review; I will move it from draft mode. Everything in the bag is included in vulkan.cppm and I have tried to be as detailed as possible in my commit messages. I have also added documentation about how to use vulkan.cppm.

When this is merged, I will post in CMake's forum about this, and hopefully the FindVulkan module is updated in the near future to add in the Vulkan-Hpp C++ module (why does everyone choose 'module' for totally different functionality?).

As for the CI, I have used some C++20 functionality (ranges and std::views range adaptors), so I think the CI pipelines should be updated.

I look forward to hearing what you guys have to say.

@sharadhr sharadhr marked this pull request as ready for review June 16, 2023 16:31
Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all those simple " using VULKAN_HPP_NAMESPACE::${name}" replacements, it's probably easier to just use that text and concatenate the name instead of calling replaceWithMap.

CMakeLists.txt Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
snippets/DispatchLoaderDefault.hpp Show resolved Hide resolved
tests/Cpp20Modules/CMakeLists.txt Outdated Show resolved Hide resolved
@asuessenbach
Copy link
Contributor

clang++-12 on Ubuntu seems to have issues with ranges... any idea what that is and how to resolve that?

@sharadhr
Copy link
Contributor Author

sharadhr commented Jun 20, 2023

clang++-12 on Ubuntu seems to have issues with ranges... any idea what that is and how to resolve that?

I think clang++12 is a bit too old for ranges, but I'm not sure. The library certainly appears to be recent enough, it's libstdc++ 12...

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't const_cast.
If you think the logic is too complex to be repeated, once for the constants and functions and once for the usings, you could introduce some preparing non-const function called in the VulkanHppGenerator constructor.

VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
VulkanHppGenerator.cpp Outdated Show resolved Hide resolved
- std::optional -> std::string::empty
- std::filesystem was unused
- 'c' prefix removed
- Constants casing fixed
- Types for constants fixed
- Constants assigned to macros
- Ordering of constants and consteval functions fixed
- Added corresponding `using` statements to `vulkan.ixx`
- Changed `consteval auto(auto)` functions into templated SFINAE `constexpr` functions
- Added newlines around macro guards
- Added signposting comments for relevant groups of `using`-statements in `vulkan.ixx`
- Guarded createInstanceUnique with macro guard
- Use m_handles.at("").commands for Funcs
- It follows the rest of the project convention; `ixx` looks really weird next to `.hpp` and `.cpp`
- CMake transparently handles any extension anyway
- Straightforward, since everything is hard-coded
- Compiler requirements
- CMake compilation
- Command-line examples
- No need for `using`, since all declarations are template specialisations of existing `std` objects
- Removed extraneous CMake version comments
- Documentation about default dynamic dispatcher with the module
- Comment updates in the source code
- Moved to after resultUsings in both vulkan.hpp and vulkan.cppm
- Also split up constexprDefinesAndUsings
- Used const_cast for constexprDefines()
- Some changes also in previous commit
- Also removed overly-clever ranges algorithms
- Removed `generateNotProtection`
- Added optional `bool` parameter to `generateProtection` for `#if !defined( ... )`
- Made C++ standard and libraries into parameters
- Removed FindVulkan call; already done
- Made all generating functions `const`
- Removed typos and extra comments
- Extracted out filtering functionality into separate functions
- Added `DefinesPartition` struct as a member variable
- Added non-const function to write to the above in `readTypeDefines`
- Removed previous implementation that made many copies
- called once at the end of the constructor
- edited comments
- CI isn't passing with them
@sharadhr
Copy link
Contributor Author

sharadhr commented Jun 27, 2023

Okay; looks like the way I have implemented the macro functions needs to change if we are to make them work for C++11. I need to remove the auto return type (change to uint32_t?) and get rid of the enable_if_t and is_integral_v, which were only added in C++14 and C++17 respectively.

I was thinking about requiring std::is_unsigned instead of std::is_integral because it makes no real sense for a version number to have a negative sign... Any thoughts? This will require that the generator change further still.

@asuessenbach
Copy link
Contributor

I think, staying with std::is_integral would be the best fit. Just don't use is_integral_v, but is_integral<T>::value.
And you don't need to append 'u' in the calls, then.

- Removed `enable_if_t` and `is_integral_v` 
- Changed `auto` return type into `uint32_t`
@asuessenbach
Copy link
Contributor

Your PR looks great now and passes all CI steps!
That is, if you say you're done, I'd be happy to merge it.

@sharadhr
Copy link
Contributor Author

I think I have nothing left to add! Thanks for assisting me with all the reviews; I think it is also ready to merge.

@asuessenbach
Copy link
Contributor

It was a pleasure to review your PR.
Having C++20 modules with Vulkan-Hpp is a great step into the future!
So, thanks a lot for your contribution.

@asuessenbach asuessenbach merged commit 6c1996f into KhronosGroup:main Jun 28, 2023
96 checks passed
@sharadhr sharadhr deleted the cpp20-modules branch June 29, 2023 08:46
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.

Add support for C++20 modules
5 participants