feat: Add C++ modules support#1530
Conversation
✅ Deploy Preview for dpp-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
e91eaa8 to
121eb9b
Compare
Mishura4
left a comment
There was a problem hiding this comment.
Thanks for the PR! Good stuff. Just gonna prefer the approach with including dpp.h in the module for maintainability, as discussed in https://discord.com/channels/825407338755653642/825411104208977952/1454456223952011427
braindigitalis
left a comment
There was a problem hiding this comment.
some module related unit tests would be good. not repeating a thousand lines of code in dpp.cppm is an absolute must, as @Mishura4 also said.
|
I've looked at the |
|
I think that in this case you might want to make a new .cpp under the unittests folder which imports dpp instead of #including it. it can have some test functions that are exported and called via standard approaches from the main unittest file. |
1909f3f to
7f6fb23
Compare
|
I think this should fix the errors we're encountering. It's with the build system configuration |
|
you cant just do -DDPP_MODULES=ON for the entire CI matrix. Some CI's use older g++ (which we support) and some use C++17. |
|
Then is it possible to disable the modules unit tests for C++17 CI tests? |
|
@braindigitalis I think the modules CI should just be in a totally separate CI. It requires Ninja, as CMake can't build modules with Makefiles. |
|
unit tests only run in one g++, which is g++-12. This is the one which is packaged. we cant and dont run unit tests on them all, as the unit tests connect to discord and we cant do this concurrently. |
|
Updated Title to match code standards |
|
any chance we can trim it down so that its easier to review? perhaps by not introducing ninja as a dependency of docs build, not changing its compiler to clang, and keeping changes to a minimum in the cmakelists etc? right now there are 53 changed files. |
braindigitalis
left a comment
There was a problem hiding this comment.
left some thoughts :-) thanks for the continued effort on this!
| }); | ||
|
|
||
| /* The interaction create event is fired when someone issues your commands */ | ||
| bot.on_slashcommand([&bot](const dpp::slashcommand_t & event) { |
There was a problem hiding this comment.
do we need to change all the examples, it makes the PR much harder to review, so many files to go through.
There was a problem hiding this comment.
I've moved these changes into a separate PR, #1604
| bytes.a | ||
| tls_syntax.a | ||
| hpke.a | ||
| $<BUILD_INTERFACE:mlspp.a> |
There was a problem hiding this comment.
really really not liking that we changed this. it took a LOT of effort to make this always build everywhere. with static and dynamic linking, on a ton of different systems. some of which CI doesnt test (e.g. static builds)
There was a problem hiding this comment.
$<BUILD_INTERFACE:> is build-time transparent, during build it expands into mlspp.a and produces the same linker command.
There was a problem hiding this comment.
then if it does the same why change it?
There was a problem hiding this comment.
@braindigitalis BUILD_INTERFACE only expands when building the library artifacts ; it's usually as a counterpart to INSTALL_INTERFACE which only expands when bringing the library from an install directory
Up until modules that was functionally the_ same as PRIVATE / PUBLIC but with modules actually PRIVATE is included when bringing module files (as they are translation units that need to be built privately into a public binary artifact, like .cpp files), even when linking an installed library. So BUILD_INTERFACE prevents this and is only used when building the library itself.
So adding BUILD_INTERFACE is correct here, even if I am not sure the author knew why.
| @@ -0,0 +1,141 @@ | |||
| /************************************************************************************ | |||
There was a problem hiding this comment.
this doesnt actually test modules, just verifies they can be linked. but, i guess, theres nothing else we can do to test that, as it is a compile-time thing.
|
OK; will look in to this. |
|
all builds failed. nlohmann doesnt need touching in this pr any more. |
There was a problem hiding this comment.
this file should not be picked up as changed at all in this pr. please merge master into the branch
| # Installation | ||
|
|
||
| include(GNUInstallDirs) | ||
| install(TARGETS dpp LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) |
There was a problem hiding this comment.
why did this get removed, is it somewhere else now?
There was a problem hiding this comment.
This install does still take place; it's done in cmake/CPackSetup.cmake, which has a richer install rule anyway; keeping it library/CMakeLists.txt just reinstalls the library a second time (with less information)
Mishura4
left a comment
There was a problem hiding this comment.
Example doesn't seem to build on Windows. Can we add it to CI somehow?
Also, please fix merge conflicts.
|
|
||
| set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -rdynamic") | ||
|
|
||
| # Create gcm.cache directory and symlink to the main build's dpp.gcm |
There was a problem hiding this comment.
Clang obtains the BMI paths through -fprebuilt-module-path which indicates where to scan, but GCC has no such option, and places its BMIs in gcm.cache/, which is why the lookup location is populated with the symlink to the actual interface.
There was a problem hiding this comment.
Why is this necessary? I've not had to do this with any of my other projects
| bytes.a | ||
| tls_syntax.a | ||
| hpke.a | ||
| $<BUILD_INTERFACE:mlspp.a> |
There was a problem hiding this comment.
@braindigitalis BUILD_INTERFACE only expands when building the library artifacts ; it's usually as a counterpart to INSTALL_INTERFACE which only expands when bringing the library from an install directory
Up until modules that was functionally the_ same as PRIVATE / PUBLIC but with modules actually PRIVATE is included when bringing module files (as they are translation units that need to be built privately into a public binary artifact, like .cpp files), even when linking an installed library. So BUILD_INTERFACE prevents this and is only used when building the library itself.
So adding BUILD_INTERFACE is correct here, even if I am not sure the author knew why.
Jaskowicz1
left a comment
There was a problem hiding this comment.
A couple comments, nothing to really reject for though
| @@ -0,0 +1 @@ | |||
| \warning D++ modules are currently only supported by D++ on g++ 15, clang/LLVM 17, and MSVC 17.6 or above. Additionally, your program has to support C++20. | |||
There was a problem hiding this comment.
Sorry to nitpick, could we word this to be D++ modules are currently experimental and are only supported by D++ on g++ 15, clang/LLVM 17, and MSVC 17.6 or above. Additionally, your program has to support C++20.
| @@ -1 +1 @@ | |||
| \warning D++ Coroutines are currently only supported by D++ on g++ 13, clang/LLVM 14, and MSVC 19.37 or above. Additionally, your program has to support C++20. | |||
| \warning D++ coroutines are currently only supported by D++ on g++ 13, clang/LLVM 14, and MSVC 19.37 or above. Additionally, your program has to support C++20. | |||
There was a problem hiding this comment.
I would remove this change, doesn't really match the capitalisation across the board but also better to keep changes to a minimum
| DONT_RUN_VCPKG: true | ||
|
|
||
| - name: Build module example | ||
| run: cmake --build build --target using_modules_example --config Release --parallel 2 |
There was a problem hiding this comment.
does this target exist? I can't see it anywhere in this PR. If it does exist, it's not being built for Windows.
This is causing CI to fail
There was a problem hiding this comment.
I asked for examples to be built in CI but yes the CI needs to be fixed
There was a problem hiding this comment.
Yeah i'm happy with that, it's just the fact that the using_modules_example target doesn't look like it exists
This pull request adds support for C++20 modules.
In order to accomplish this, some constants were changed to using external linkage.
I have also updated nlohmann/json to version 3.12.0, as the older version declares some symbols as internal.Updating nlohmann/json has been moved to #1596.Code change checklist