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

C++20 module improvements #1815

Closed
sharadhr opened this issue Feb 27, 2024 · 10 comments · Fixed by #1932
Closed

C++20 module improvements #1815

sharadhr opened this issue Feb 27, 2024 · 10 comments · Fixed by #1932

Comments

@sharadhr
Copy link
Contributor

sharadhr commented Feb 27, 2024

There are a few things I'd like to improve about vulkan.cppm, stemming from my own experience, and bug reports around.

I'd like to spur some discussion on this, and if it's worthwhile, start a relevant PR.

@g-martin772
Copy link

Making the vulkan_hpp module work with import std; would be great!

@sharadhr
Copy link
Contributor Author

sharadhr commented Mar 6, 2024

@asuessenbach and @theHamsta, do you have any input on this? Thanks!

@asuessenbach
Copy link
Contributor

Please sorry for the delay.
I think, you're right, using import std seems to be the right move. Would be great if you could start a PR on it.

@stripe2933
Copy link

stripe2933 commented Mar 22, 2024

Hello, I created a draft commit for enable import std conditionally using VULKAN_HPP_USE_STD_MODULE CMake option. It works with my Clang 17 + custom libc++ build, but I have no idea that it works on other environment (also it may breaks the usage for non-module usage). Are there anything I should do before the PR? Here's the commit in the forked repository.

@sharadhr
Copy link
Contributor Author

Thanks @stripe2933! I've personally been putting off this because CMake 3.30 (probably releasing in a couple of months; 3.29 is already at rc4) will have full support for import std without our having to do much: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9337

@stripe2933
Copy link

Oh, I was one step behind. That sounds great. It will fix the module usage with MSVC.

@stripe2933
Copy link

Is there any progress on this?

@sharadhr
Copy link
Contributor Author

sharadhr commented Jul 23, 2024

Sorry, been a bit overwhelmed with other stuff and the day job.

I have some preliminary work done (very similar to yours, @stripe2933, because the gist of it boils down to writing import std; somewhere, and guard all the #include <stdheader> behind a new option macro). I'm happy to leave the PR up and open, but I don't think it should be merged yet, because there are a whole bunch of catch-22s.

  • CMake's import std support while available in CMake 3.30 is still considered 'experimental' and requires a variable with the UUID to be defined before the project is configured, either in a preset or in a CMake list file, before a call to project().
  • MSVC with VS 2022 17.10 still ICEs when building vulkan.cppm; this will only be resolved in 17.11.
  • If I try to use Clang 18, CMake somehow complains that the only stdlib supported is libc++, which is very strange.
  • Compiler support for both C++ modules and the import std; module is still... lacklustre, honestly.

Frankly this is a compiler bug more than anything; users should be able to mix #include <stdheader> and import std; freely, but compiler support is just not there yet. And personally I don't want to either clutter up the top-level CMakeLists with the UUID variable, or force users to define it. This was a bit of a headache last year with the C++20 module work, and I was very glad when it was made final in CMake 3.28.

@stripe2933
Copy link

Sorry for late reply. I really appreciate your hard work!

  • If I try to use Clang 18, CMake somehow complains that the only stdlib supported is libc++, which is very strange.

Are you in the Linux environment? In Linux, Clang uses libstdc++ by default. You have to pass -DCMAKE_CXX_FLAGS=-stdlib=libc++ -DCMAKE_EXE_LINKER_FLAGS=-stdlib=libc++ -lc++abi for enable libc++.

  • MSVC with VS 2022 17.10 still ICEs when building vulkan.cppm; this will only be resolved in 17.11.

My project uses vulkan.cppm and standard library module, and it can be compiled in both MSVC 17.10 and Clang 18. I hope this may help to you.

Here are my some observations for MSVC ICE for vulkan.cppm:

  • Are you defined VULKAN_HPP_NO_SMART_HANDLE macro definition? As mentioned in README, without this macro compiler will be fall into ICE.
  • I cannot certain, but vk::ArrayProxy and vk::ArrayProxyNoTemporaries looks very problematic for MSVC. Sometimes range-passing constructor of Vulkan handles with enhanced mode not working, and emits error C2028: struct/union member must be inside a struct/union. Looks like MSVC cannot accept the heavily templated code.
  • Very strange missing operator""sv error. Including <string_view> header will fix it, but the errorness file doesn't have any operator""sv usage... But it seems this can be fixed when using std module.

@sharadhr
Copy link
Contributor Author

sharadhr commented Aug 5, 2024

Thanks for the response!

Are you in the Linux environment?

No; I'm using Windows with Clang 18 (clang.exe). I haven't used Linux in a while, so I really have to depend on the CI to ensure everything works...

  • Are you defined VULKAN_HPP_NO_SMART_HANDLE macro definition? As mentioned in README, without this macro compiler will be fall into ICE.

Good point. I personally don't use any of the smart handles (I prefer the vk::raii stuff), but I'd really rather wait for the fix to land. 17.11 is in Preview 5 and I suspect it will see a few more preview cycles and release in a few weeks.

  • Very strange missing operator""sv error. Including <string_view> header will fix it, but the errorness file doesn't have any operator""sv usage... But it seems this can be fixed when using std module.

Indeed, I've seen this too. I've also seen issues with <compare>, and this is partly the motivation for this issue and PR.

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 a pull request may close this issue.

4 participants