-
Notifications
You must be signed in to change notification settings - Fork 3
Add support for C++ modules #1
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
Conversation
jimmyorourke
left a comment
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.
Thanks!
modules/CMakeLists.txt
Outdated
|
|
||
| target_compile_features(plotlypp_module PUBLIC cxx_std_20) | ||
|
|
||
| target_include_directories(plotlypp_module PUBLIC |
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.
Instead of setting include dirs again, let's link against the plotlypp target.
target_link_libraries(plotlypp_module PUBLIC plotlypp)
(I'm wondering if we might need the plotlypp target added to the install export set back in the main cmakelists)
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.
I have now added the install directives, export set, etc for the plotlypp target to the main cmakelists if you rebase
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.
Actually I've changed my mind and I think we can make this all much simpler.
I don't think we need a separate CMakeLists, or separate modules target.
In the main CMakeLists, we can bump the min version to 3.28. Then we could do
if(PLOTLYPP_BUILD_MODULES)
# modules BMI target
add_library(plotlypp)
target_sources(...
else()
# Header only
add_library(plotlypp INTERFACE)
endif()
We set the cxx_standard conditionally as well, change the linking to public from interface, add the modules export section to the install target (that I just added), and hopefully it should just work...
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.
OK, I've applied this change now.
| export module plotlypp; | ||
|
|
||
| export namespace plotlypp { | ||
| using plotlypp::Figure; |
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.
For now this is fine, but you've got me thinking about whether we should auto generate this file with the rest of the source generation, or create and apply an export macro at source generation time.
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.
I think there are some known ways of doing this, such as MSVC's EXPORT_API macro which expands to export when #included into a module.
|
I don't have proper unit tests yet for this project, part due to its nature and part due to its stage, (JSON comparisons are probably the best bet), but it might be good to throw together a quick example target that we can ensure compiles. The CI builds have a c++23 variant in which we could enable the modules option. |
This pull request adds support for C++ modules, enabled in CMake with
PLOTLYPP_BUILD_MODULES. It pulls in all headers of the library, including<plotlypp/traces/*.hpp>.