Use import std; guarded by macro

[sharadhr]

Resolves #1815. Work in progress.

[qbojj]

There is one problem with this approach:
as stated in http://eel.is/c++draft/cpp#import-3:

If a pp-import is produced by source file inclusion (including by the rewrite produced when a #include directive names an importable header) while processing the group of a module-file, the program is ill-formed.

so imports in #included files are ill-formed.

[sharadhr]

Ah... I was trying to put everything in the headers; looks like I'll have to take a leaf from @stripe2933's approach and put the imports in the .cppm itself, guarded by the macro. I'll rework this in the evening.

[sharadhr]

There's a bit of a problem here and I'd like some thoughts.

I'm defining the macro VULKAN_HPP_CPP_STD_MODULE in vulkan_hpp_macros.hpp based on the platform and compiler characteristics. This means we need #include <vulkan/vulkan_hpp_macros.hpp> in the global module fragment, i.e. between module; and export vulkan_hpp;.

Then, in several of the headers (especially vulkan.hpp), I selectively disable #include <stdheader> based on the presence of VULKAN_HPP_CPP_STD_MODULE.

Now, the import std; needs to appear before the various #include statements; otherwise the compiler complains that std is not available.

However, import statements can't appear in the global module fragment (this is what @stripe2933 did in his fork), and we have a Catch-22 situation. It can be resolved with import <vulkanheader> but header units are neither widely supported nor recommended.

[stripe2933]

I also worked to port the magic_enum's module, and I think this file shows the solution:

You can include headers below to the export module module_name; by enclosing them with extern "C++". Therefore, include std first, and include the Vulkan headers will solve this problem

[sharadhr]

I'm not sure that is a good resolution: this blog post says:

C++ Modules is almost entirely agnostic to the existence of the preprocessor. What you’ve just done in the above code is cause every single symbol in SDL.h to become attached to my_game (with module linkage). Obviously, your module does not own the contents of SDL.h.

However, the standard does allow for import statements in the global module fragment:

module.global.frag

[Note 1: Prior to phase 4 of translation, only preprocessing directives can appear in the declaration-seq ([cpp.pre]). — end note]

cpp.pre

A preprocessing directive consists of a sequence of preprocessing tokens that satisfies the following constraints: At the start of translation phase 4, the first token in the sequence, referred to as a directive-introducing token, begins with the first character in the source file (optionally after whitespace containing no new-line characters) or follows whitespace containing at least one new-line character, and is
(1.1)
a # preprocessing token, or
(1.2)
an import preprocessing token immediately followed on the same logical source line by a header-name, <, identifier, string-literal, or : preprocessing token, or
(1.3)
a module preprocessing token immediately followed on the same logical source line by an identifier, :, or ; preprocessing token, or
(1.4)
an export preprocessing token immediately followed on the same logical source line by one of the two preceding forms.

So I believe it is safer to put the import in the global module fragment.

[qbojj]

There is also an option to define something like VULKAN_HPP_EXPORT to export only when compiling the module and using it with every exported declaration. With this approach, the module would look something like:

module;
#define VULKAN_HPP_EXPORT export

#include <vulkan/vulkan.h>
#include <vulkan/vulkan_hpp_macros.hpp>

#if !VULKAN_HPP_USE_STD_MODULE
// includes
#endif

export module vulkan_hpp;

#if VULKAN_HPP_USE_STD_MODULE
import std;
import std.compat;
#endif

#include <vulkan/vulkan.hpp>
// other vulkan includes

I've tried to go with this approach, but I had problems with vk::DispatchLoaderDynamic as it is defined in vulkan_hpp_macros.hpp, but from my initial tries It should work.

[sharadhr]

I think your initial claim is misconstrued—you've quoted the standard:

http://eel.is/c++draft/cpp#import-3:

If a pp-import is produced by source file inclusion (including by the rewrite produced when a #include directive names an importable header) while processing the group of a module-file, the program is ill-formed.

In this context pp-imports are 'header units'. This says that headers are themselves not allowed to import <headerunit>;. But std is not header; it is a named module. Headers themselves are allowed to import named modules. I think I'll revert to my original implementation, which is cleaner.

[qbojj]

Now that I look at it, You are right. I'm sorry for the confusion

[sharadhr]

Actually, I misconstrued that myself too. That line refers to a module 'group', which is the part between export module vulkan_hpp; and the end of the file, or module :private;. So either way, both import <header>; and import namedmodule; statements can appear in header files.

[sharadhr]

I'm not entirely sure why the CI tasks are failing so badly... @asuessenbach, any thoughts?

[asuessenbach]

I'm not entirely sure why the CI tasks are failing so badly... @asuessenbach, any thoughts?

Just one thing: the CI on MacOS needed some update (#1942). But I don't see what might have happened on the other CIs. Don't you see the build issues on your local machine as well?

[sharadhr]

Now that you mention it, I do see that build failure on MSVC, but I'm not sure it's relevant to this PR. In particular,

Vulkan-Hpp/tests/UniqueHandleDefaultArguments/UniqueHandleDefaultArguments.cpp

Line 29 in d86b49c

auto uniqueSurface = vk::UniqueSurfaceKHR( surface, vk::Instance() );

needs to be:

- auto         uniqueSurface = vk::UniqueSurfaceKHR( surface, vk::Instance() ); 
+ auto         uniqueSurface = vk::UniqueSurfaceKHR( vk::SurfaceKHR( surface ), vk::Instance() ); 

because it seems the vk::SurfaceKHR( VkSurfaceKHR ) constructor seems to have been marked explicit (which seems like a bug, this should only happen on 32-bit platforms).

[asuessenbach]

because it seems the vk::SurfaceKHR( VkSurfaceKHR ) constructor seems to have been marked explicit (which seems like a bug, this should only happen on 32-bit platforms).

And that's the case with the main branch, but seems to be not the case with your branch.
Determining, if it should be explicit depends on some define from vulkan_core.h (VK_USE_64_BIT_PTR_DEFINES). Do you miss to include that, somehow?

[sharadhr]

Right, I've resolved it. If the macros in vulkan_hpp_macros.hpp depend on vulkan.h then it only makes sense to #include the latter in the former, which I've done.

[sharadhr]

Ready to merge.

[asuessenbach]

Looks great now, thanks a lot again for your contribution!

Could you please also generate the vulkansc files (by running the VulkanHppGenerator with -api=vulkansc as argument)?

[sharadhr]

@stripe2933 while the CI completes, if you could test this out that'd be great. Refer to https://github.com/KhronosGroup/Vulkan-Hpp/blob/41db2194537c8863927735e2500e326e65e5a0a8/tests/CppStdModule/CMakeLists.txt if you'd like to see how to set it up.

[stripe2933]

@sharadhr I'm sorry, but I'm using custom MoltenVK build configuration based on 1.2.8 version, and I cannot upgrade the Vulkan SDK to 1.3.290 because of the MoltenVK 1.2.9 issue.

I changed static_assert( VK_HEADER_VERSION == 292, "Wrong VK_HEADER_VERSION!" ); code by replacing 292 to 283 but it emits several errors (due to the incompatibility with vulkan.h). Is there any workaround for this?

[asuessenbach]

Is there any workaround for this?

I don't think so.
You could try to store the changes of this PR as a patch and apply that patch to the 283-build (https://github.com/KhronosGroup/Vulkan-Hpp/releases/tag/v1.3.283). But I don't know, if that actually works.

[stripe2933]

I tested this in Clang 18 by manually updating the relevant portions of the header and module files, and everything worked seamlessly. Great work, @sharadhr! If you have time, I have a couple of questions:

1. I noticed that I didn't moved <vulkan/vulkan.h> from vulkan/vulkan.hpp to vulkan/vulkan_hpp_macros.hpp file, yet it compiled successfully. Could you clarify why this file is moved? I think this change might not only add noticeable build overhead for every TU to include <vulkan/vulkan_hpp_macros.hpp> file, but also violate the meaning of the header name (most users will expect this header only contains macros about Vulkan-Hpp).
2. Could you explain the rationale behind defining VULKAN_HPP_STD_MODULE and VULKAN_HPP_STD_COMPAT_MODULE as std and std.compat, respectively (and not just directly using them)? Since these are reserved identifiers in the C++ specification, I'm curious about the reasoning for their usage.

[sharadhr]

@stripe2933 thanks for testing! To answer your questions:

1. I moved #include <vulkan/vulkan.h> to vulkan_hpp_macros.hpp because the latter has the following few lines:

   // 32-bit vulkan is not typesafe for non-dispatchable handles, so don't allow copy constructors on this platform by default.
   // To enable this feature on 32-bit platforms please #define VULKAN_HPP_TYPESAFE_CONVERSION 1
   // To disable this feature on 64-bit platforms please #define VULKAN_HPP_TYPESAFE_CONVERSION 0
   #if ( VK_USE_64_BIT_PTR_DEFINES == 1 )
   #  if !defined( VULKAN_HPP_TYPESAFE_CONVERSION )
   #    define VULKAN_HPP_TYPESAFE_CONVERSION 1
   #  endif
   #endif

   and in turn, VK_USE_64_BIT_PTR_DEFINES == 1 is defined in vulkan_core.h (which is included in vulkan.h). This means vulkan_hpp_macros.hpp depends on vulkan_core.h.

   I could refactor so that the macros file only includes the core header, but there is no escaping having at least this macro in it (otherwise the macros file would be incomplete).

2. I followed the style of the rest of the project where keywords and headers are re-defined with VULKAN_HPP_* (see what has been done with constexpr and std::expected, for instance). It is always possible that these might change, so it is easier to set up these macros once and re-define them to something else if necessary. Really a matter of style, I feel.

[stripe2933]

Thank you for the detailed explanation. Regarding the first question, if this issue only affects 32-bit operating systems, how about remain the #include <vulkan/vulkan.h> line in the vulkan.hpp file for 64-bit environments? Since most operating systems are 64-bit, this could help reduce the overhead.

[sharadhr]

The entire reasoning is as follows: in vulkan.hpp we have standard header files that need to be conditionally included (or the std module imported instead), based on the macro. That macro is in vulkan_hpp_macros.hpp. That in turn depends on vulkan_core.h.

Additionally vulkan_hpp_macros.hpp may be included on its own as well, and every macro in it must be well-defined or at most dependent on a user-configured value.

If you can see a way around this, though, I'll be happy to refactor it! I fully understand because vulkan_core.h is not a light header in any way, it's ~20K lines of code that needs to be repeated in every TU that needs it.

What I could do is copy the test and definition for VK_USE_64_BIT_PTR_DEFINES into vulkan_hpp_macros.hpp, if @asuessenbach is happy with that.

[stripe2933]

What I could do is copy the test and definition for VK_USE_64_BIT_PTR_DEFINES into vulkan_hpp_macros.hpp, if @asuessenbach is happy with that.

I think this is the best way to around it. As Vulkan specification says, the vulkan_core.h header allows the VK_USE_64_BIT_PTR_DEFINES definition to be overridden by the application. Re-defining the macro outside the vulkan_core.h file seems not dangerous anyway.

[stripe2933]

Not really, will it? <version> is already present in the macros header.

I missed that by mistake. You’re right.

---

I don’t see any other problems. Looks good to me 👍

[asuessenbach]

Looks great, overall!
Just one issue I'd like to have resolved.

[sharadhr]

Reworked again! @asuessenbach, let me know if you'd rather I squashed the commits (unless you can do that when you merge).

[stripe2933]

After applying this patch, I got weird build error when I construct std::string_view from vk::ArrayWrapper1D<char, 256> (e.g. from vk::PhysicalDevice::enumerateDeviceExtensionProperties).

/usr/local/include/vulkan/vulkan.hpp:153:46: error: call to function 'strnlen' that is neither visible in the template definition nor found by argument-dependent lookup
153 | return std::string_view( this->data(), strnlen( this->data(), N ) );

I'm using macOS 15.0 and Homebrew Clang 18.1.6. Could you check if this build error is reproduced?

[stripe2933]

I just realized that strnlen isn't a part of the C standard. This function really looks like it should be standard (^^;;). The function is not exported in libc++ module. I think I should replace it with strnlen_s or std::find.

[sharadhr]

I just realized that strnlen isn't a part of the C standard. This function really looks like it should be standard (^^;;). The function is not exported in libc++ module. I think I should replace it with strnlen_s or std::find.

Just reproduced with MSVC 19.40 (VS 2022 17.11). strnlen_s doesn't work either; in fact none of the <cstring> or <string.h> functions appear to be exported. For now I have used #include <string.h> to mitigate this.

[sharadhr]

I think there is nothing further to be done; ready to merge.

[asuessenbach]

@sharadhr Thanks a lot for your ongoing and great support here! Makes Vulkan-Hpp somewhat more future-proved.