Skip to content

Conversation

@mikomikotaishi
Copy link
Contributor

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>.

Copy link
Owner

@jimmyorourke jimmyorourke left a comment

Choose a reason for hiding this comment

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

Thanks!


target_compile_features(plotlypp_module PUBLIC cxx_std_20)

target_include_directories(plotlypp_module PUBLIC
Copy link
Owner

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)

Copy link
Owner

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

Copy link
Owner

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...

Copy link
Contributor Author

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;
Copy link
Owner

@jimmyorourke jimmyorourke Jan 21, 2026

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.

Copy link
Contributor Author

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.

@jimmyorourke
Copy link
Owner

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.

@jimmyorourke jimmyorourke merged commit fbef4cf into jimmyorourke:main Jan 22, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants