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.

Sign up for GitHub

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

Jump to bottom

C++20 named module documentation updates #1767

Merged
merged 4 commits into from
Jan 8, 2024
Merged

C++20 named module documentation updates #1767

merged 4 commits into from
Jan 8, 2024

Conversation

sharadhr
Copy link
Contributor

@sharadhr sharadhr commented Jan 5, 2024

  • Fix CMake instructions for the C++20 named module
  • Provided examples on module config
  • Elaborated on dynamic dispatcher setup for the module

Resolves #1753.

@sharadhr
Named module updates
388af4e 
- Fix CMake instructions for the module
- Provided examples on module config
- Elaborated on the dynamic dispatcher
@sharadhr sharadhr force-pushed the documentation-updates branch from 178a37c to 388af4e Compare January 5, 2024 00:58
monamimani
monamimani reviewed Jan 5, 2024
View reviewed changes
Copy link

@monamimani monamimani left a comment

Choose a reason for hiding this comment

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

Beside that little comment everything looks fine. :)

README.md Show resolved Hide resolved
@sharadhr sharadhr changed the title C++20 named module updates C++20 named module documentation updates Jan 5, 2024
@sharadhr
Update README.md
3c6c0bf 
- Rearranged warning
- Removed subdirectory from `BASE_DIRS` 
- Added subdirectory to `FILES`
- Added more information for dynamic loader CMake setup with module
@sharadhr sharadhr force-pushed the documentation-updates branch from b2dbadc to 3c6c0bf Compare January 5, 2024 20:17
@monamimani
Copy link

No worries :)
I also added for good measure
set(CMAKE_CXX_STANDARD 20)

and I also needed this on my side to make things compile if not you can't include the macros header and the compilation of the cppm fails.

target_include_directories(VulkanHppModule
PUBLIC
"${Vulkan_INCLUDE_DIR}"
)

@sharadhr
Copy link
Contributor Author

sharadhr commented Jan 5, 2024 •
edited
Loading

and I also needed this on my side to make things compile if not you can't include the macros header and the compilation of the cppm fails.

target_include_directories(VulkanHppModule
PUBLIC
"${Vulkan_INCLUDE_DIR}"
)

Hmm, I didn't need that. Linking to Vulkan::Vulkan should be sufficient, as it asks CMake to include the Vulkan header directory, too.

@monamimani
Copy link

monamimani commented Jan 5, 2024 •
edited
Loading

You are right, Vulkan::Vulkan was linking on an other target. Linking it to VulkanHppModule make it so we don't need the target_include_directories.
So all is good to go I think.

Thanks!

monamimani
monamimani approved these changes Jan 5, 2024
View reviewed changes
Copy link

@monamimani monamimani left a comment

Choose a reason for hiding this comment

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

All good!

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
theHamsta
theHamsta reviewed Jan 6, 2024
View reviewed changes
README.md Show resolved Hide resolved
@sharadhr
Add minimum required Vulkan version check in the CMake module example
53e3315 
Add minimum required Vulkan version check in the CMake module example
@sharadhr sharadhr force-pushed the documentation-updates branch from 9a1dfbb to 53e3315 Compare January 7, 2024 21:19
@asuessenbach
Copy link
Contributor

@sharadhr and @monamimani Thanks a lot for your great support here! The result looks good.
If you both agree to the current state of this PR, I will merge it.

@sharadhr
Copy link
Contributor Author

sharadhr commented Jan 8, 2024

@sharadhr and @monamimani Thanks a lot for your great support here! The result looks good. If you both agree to the current state of this PR, I will merge it.

LGTM!

@monamimani
Copy link

@sharadhr and @monamimani Thanks a lot for your great support here! The result looks good.
If you both agree to the current state of this PR, I will merge it.

Yep let's go 😃

@asuessenbach asuessenbach merged commit e843529 into KhronosGroup:main Jan 8, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theHamsta theHamsta theHamsta left review comments

@monamimani monamimani monamimani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix for Cmake for vulkan_hpp c++ modules
4 participants
@sharadhr @monamimani @asuessenbach @theHamsta