Rewrite modules following the new recommended Boost practice #196

apolukhin · 2025-04-07T14:10:53Z

Changes:

#include <boost/pfr... is now implicitly changed into import boost.pfr if the modules are supported
CI now tests modules on Ubuntu 24.04
Renamed module to boost.pfr
CMakeLists.txt now uses modules for Boost::pfr target if modules are supported
All the library internals now have unconditional module level linkage. 1) allows users to mix #include <boost/pfr... and import boost.pfr in user code without ODR-violations.

Significant differences from https://anarthal.github.io/cppblog/modules3:

PFR uses a separate macro for import std; / includes. This allows using boost.pfr module in C++20 and even without usable std module.

apolukhin · 2025-04-07T17:54:23Z

@anarthal I'd appreciate comments on this PR on modules (as you proposed at boostorg/core#191 (comment))

coveralls · 2025-04-07T20:14:38Z

Pull Request Test Coverage Report for Build 14447165733

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 12675824451:	0.0%
Covered Lines:	407
Relevant Lines:	407

💛 - Coveralls

anarthal · 2025-04-08T10:55:06Z

Note that 2) seems to make it safe to change all the library internals to have module level linkage and still allows users to mix #include <boost/pfr... and import boost.pfr in user code without ODR-violations. I'll try to implement it next time

Have you tried MSVC? My experience is that clang allows for almost anything, with MSVC being much more restrictive.

anarthal · 2025-04-08T10:56:50Z

.github/workflows/ci.yml

          ./b2 -d0 headers
          ./b2 variant=debug tools/inspect/build

+      - name: Run modules tests


I'd strongly advise to add a job covering modules under MSVC, since it's much more restrictive than clang. You can have a look at my mp11/charconv proposal for some inspiration.

anarthal

What problems did you encounter in the tests that you commented out? I might be able to help with them.

anarthal · 2025-04-08T11:10:27Z

include/boost/pfr/core.hpp


+#include <boost/pfr/tuple_size.hpp>
+
+#if defined(BOOST_USE_STD_MODULE)


Since PFR is the only library supporting this atm, I'd consider renaming the macro to BOOST_PFR_USE_STD_MODULE

anarthal · 2025-04-08T11:13:00Z

include/boost/pfr/core.hpp

+
+#if defined(BOOST_USE_STD_MODULE)
+import std;
+#else


I'd consider replacing this ifdef + includes by either a file like this:

https://github.com/anarthal/mp11/blob/feature/cxx20-modules/include/boost/mp11/detail/std/tuple.hpp

or follow this approach: boostorg/charconv#255 (comment)

I'd rather leave it as is for now. Maybe I'll change it in the next PR

anarthal · 2025-04-08T11:27:31Z

.github/workflows/ci.yml

+          cd ../boost-root/libs/pfr
+          mkdir build_module
+          cd build_module
+          cmake -DBOOST_USE_MODULES=1 -DBUILD_TESTING=1 -GNinja -DCMAKE_CXX_COMPILER=clang++-19 -DCMAKE_CXX_FLAGS=-I../../../ ..


If you're going to support building with and without import std, you need to add a CI build for each config/compiler pair. I think you're missing an include in the import std configuration.

anarthal · 2025-04-08T11:29:34Z

include/boost/pfr/detail/fields_count.hpp

 #include <boost/pfr/detail/unsafe_declval.hpp>

-#ifdef BOOST_PFR_HAS_STD_MODULE
+#if defined(BOOST_USE_STD_MODULE)


I don't think you will get macro definitions like __cpp_lib_is_aggregate without an include (like <version>). #include <version> is safe to use in general, and can be mixed with imports without problem, but you will need to guard it (it's C++20 and above).

Same applies for SIZE_MAX. You can replace this by (std::numeric_limits<std::size_t>::max)() and avoid the problem.

anarthal · 2025-04-08T11:32:44Z

include/boost/pfr/detail/fields_count.hpp

+#if defined(BOOST_USE_STD_MODULE)
 import std;
 #else
 #include <climits>      // CHAR_BIT


You won't get CHAR_BIT with this structure. You will need to include the header even in modular builds. IIRC #include <climits> caused problems under MSVC when mixed with imports, but #include <limits.h> is okay.

I'll just use std::numeric_limits<unsigned char>::digits instead

anarthal · 2025-04-08T11:37:05Z

include/boost/pfr/detail/possible_reflectable.hpp

 #include <boost/pfr/traits_fwd.hpp>

-#ifdef BOOST_PFR_HAS_STD_MODULE
+#if defined(BOOST_USE_STD_MODULE)


__cpp_lib_is_aggregate

anarthal · 2025-04-08T11:39:10Z

include/boost/pfr/config.hpp

 #endif

 #ifndef BOOST_PFR_USE_STD_MAKE_INTEGRAL_SEQUENCE
+#   if defined(BOOST_USE_MODULES)


Maybe you can attempt to include <version> to get __GLIBCXX__? Although that makes the include heavier.

anarthal · 2025-04-08T14:05:46Z

CMakeLists.txt

+        FILE_SET modules_public TYPE CXX_MODULES FILES
+            ${CMAKE_CURRENT_LIST_DIR}/modules/pfr.cppm
+    )
+    if (CXX_MODULE_STD)


I don't think this works like this. If I understood CXX_MODULE_STD correctly, this is a property of targets, not a global variable indicating whether import std is supported or not.

To match the rest of Boost, I'd make import std the default when BOOST_USE_MODULES is defined. Then, if the user sets some CMake option (like BOOST_PFR_DISABLE_IMPORT_STD) fall back to includes. Since stdlib is the main source of compile times, I'd expect most of the people using modules because of compile times to also use import std.

You need to test these workflows to avoid this kind of errors. We commonly test installing the project and consuming it with find_package, and consuming the project directly via add_subdirectory. Have a look at the mp11 CI workflows for examples.

You are right, I have to check the CMAKE_CXX_COMPILER_IMPORT_STD variable instead of the CXX_MODULE_STD property

anarthal · 2025-04-08T14:06:20Z

CMakeLists.txt

-if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.28.0" AND BUILD_MODULE)
-    add_subdirectory(module)
+    target_compile_definitions(boost_pfr PUBLIC BOOST_USE_MODULES)
+    target_include_directories(boost_pfr PUBLIC ${CMAKE_CURRENT_LIST_DIR}/include)


This should be just include, rather than ${CMAKE_CURRENT_LIST_DIR}/include

anarthal · 2025-04-08T14:08:03Z

test/core/run/core17_generated.cpp


 int main() {
-#if BOOST_PFR_USE_CPP17
+#if BOOST_PFR_USE_CPP17 && !defined(BOOST_USE_MODULES)  // TODO: fix for BOOST_USE_MODULES


what's the problem with this one?

It uses boost::pfr::detail that is an implentation detail and is not exported from module

Ah! I see. I've workarounded these in Charconv by just using includes. It may make sense to leave them as you did, too. I tried creating an internal-only module that exported the detail functions to test, but the approach doesn't map very well to CMake, and it tends to either end up creating an extra static library that ends up getting installed (not good) or just refusing to build at all.

apolukhin added 8 commits April 7, 2025 17:10

Rewrite modules to the new recommended Boost practice

5ec603c

fixes and tests

3792637

fix

56a0f2d

fix

709a25d

more fixes

6be268d

CI fixes

8971c38

fix attempt

0c8d6e4

CI fixes

282f645

anarthal reviewed Apr 8, 2025

View reviewed changes

apolukhin added 8 commits April 10, 2025 17:41

Review fixes (more to come)

b120be8

CI fixes

d3b3904

Attempt to reduce boilerplate

34a9890

CI fix

622e4c9

CI fix attempt

9e43d26

fix

5182bda

CI fixes

bb1bc0a

Force non-global module fragment usage for PFR

1a996e9

apolukhin changed the title ~~Rewrite modules to the new recommended Boost practice~~ Rewrite modules following the new recommended Boost practice Apr 16, 2025

apolukhin merged commit 5a48d74 into develop Apr 16, 2025
12 checks passed

apolukhin deleted the modules-take-two branch April 16, 2025 06:16


		#include <boost/pfr/tuple_size.hpp>

		#if defined(BOOST_USE_STD_MODULE)

Rewrite modules following the new recommended Boost practice #196

Rewrite modules following the new recommended Boost practice #196

Uh oh!

Conversation

apolukhin commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apolukhin commented Apr 7, 2025

Uh oh!

coveralls commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 14447165733

Details

💛 - Coveralls

Uh oh!

anarthal commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anarthal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

apolukhin commented Apr 7, 2025 •

edited

Loading

coveralls commented Apr 7, 2025 •

edited

Loading