-
Notifications
You must be signed in to change notification settings - Fork 341
Re-introduce support for std::expected fixing issue #2375 #2399
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
Re-introduce support for std::expected fixing issue #2375 #2399
Conversation
|
Thanks for providing this little PR to re-introduce support for But both files you've modified, Moreover, I think it needs to be a bit more complex. For example, there are functions with more than just |
This commit reverts the manual editing of the Vulkan Hpp generated header files, now that the snippets were updated. Re-generating the vulkan headers with the updated snippets should produce these same changes now.
|
@asuessenbach I've updated the PR to remove the manual editing of the vulkan.hpp and vulkan_hpp_macros.hpp and edited the snippets instead. Is it usually required to also run the generator and update the vulkan headers are part of check-in? or would that be done anyway as part of a release? The reason I ask, is that i don't see any hard restriction on clang-format versioning so if individual contributors are running the codegen, that will result in a buch of different commits with different formatting of these generated files creating a lot of noise. I'd expect you want either the project lead or an automated build action to be the one to actually generate and commit the updated vulkan headers. Let me know if this needs anymore changes. Thanks 😄 |
Yep, that would be great. |
You're right. Up to now, that hasn't been a big issue, though. |
|
@asuessenbach Okay, I've generated the vulkan.hpp and vulkan_hpp_macros.hpp headers using VulkanHppGenerator and clang-format 18.1.7 |
| using type = VULKAN_HPP_EXPECTED<T, Result>; | ||
| #else | ||
| using type = ResultValue<T>; | ||
| #endif |
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.
That is, the type is either VULKAN_HPP_EXPECTED<T, Result> or ResultValue<T>, mostly depending on the C++ version?
Do those two types look similar enough, that your compiler won't realize a difference where that type is actually used? If not, your code base would fail when switching to C++23.
That is, maybe you need some additional VULKAN_HPP_USE_STD_EXPECTED, or so, as an additional guard in snippets/MacrosHppTemplate.hpp?
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.
Sure, I've added VULKAN_HPP_USE_STD_EXPECTED which now needs to be defined before including vulkan hpp headers to enable use of std::expected type. Otherwise it falls back to using ResultValue
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.
And finally, that VULKAN_HPP_USE_STD_EXPECTED should be mentioned as an option in CMakeLists.txt and the README.md.
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.
Done
This PR re-inroduces support for std::expected as a ResultValueType to fix #2375