Skip to content

Rewrite modules following the new recommended Boost practice #196

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

Merged
apolukhin merged 16 commits into from
Apr 16, 2025
Merged

Rewrite modules following the new recommended Boost practice #196

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5ec603c
Rewrite modules to the new recommended Boost practice
apolukhin Apr 7, 2025
3792637
fixes and tests
apolukhin Apr 7, 2025
56a0f2d
fix
apolukhin Apr 7, 2025
709a25d
fix
apolukhin Apr 7, 2025
6be268d
more fixes
apolukhin Apr 7, 2025
8971c38
CI fixes
apolukhin Apr 7, 2025
0c8d6e4
fix attempt
apolukhin Apr 7, 2025
282f645
CI fixes
apolukhin Apr 7, 2025
b120be8
Review fixes (more to come)
apolukhin Apr 10, 2025
d3b3904
CI fixes
apolukhin Apr 10, 2025
34a9890
Attempt to reduce boilerplate
apolukhin Apr 14, 2025
622e4c9
CI fix
apolukhin Apr 14, 2025
9e43d26
CI fix attempt
apolukhin Apr 14, 2025
5182bda
fix
apolukhin Apr 14, 2025
bb1bc0a
CI fixes
apolukhin Apr 14, 2025
1a996e9
Force non-global module fragment usage for PFR
apolukhin Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ jobs:
- toolset: clang-15
cxxstd: "03,11,14,17,20"
os: ubuntu-22.04
- toolset: clang-19
cxxstd: "20,23"
os: ubuntu-24.04
install: clang-19 clang-tools-19
# - toolset: clang
# cxxstd: "03,11,14,17,2a"
# os: macos-10.15
Expand Down Expand Up @@ -87,6 +91,17 @@ jobs:
./b2 -d0 headers
./b2 variant=debug tools/inspect/build

- name: Run modules tests

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

if: ${{matrix.toolset == 'clang-19'}}
run: |
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../../../ ..

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

cmake --build . && ctest -V
cd ..
rm -rf build_module

- name: Run tests
run: |
cd ../boost-root
Expand Down
40 changes: 32 additions & 8 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,44 @@
# Distributed under the Boost Software License, Version 1.0.
# https://www.boost.org/LICENSE_1_0.txt

cmake_minimum_required(VERSION 3.5...3.16)
cmake_minimum_required(VERSION 3.5...3.31)

project(boost_pfr VERSION "${BOOST_SUPERPROJECT_VERSION}" LANGUAGES CXX)

add_library(boost_pfr INTERFACE)
add_library(Boost::pfr ALIAS boost_pfr)

target_include_directories(boost_pfr INTERFACE include)
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.28.0" AND BOOST_USE_MODULES)
add_library(boost_pfr)
target_compile_features(boost_pfr PUBLIC cxx_std_20)
target_sources(boost_pfr PUBLIC
FILE_SET modules_public TYPE CXX_MODULES FILES
${CMAKE_CURRENT_LIST_DIR}/modules/pfr.cppm
)
if (CXX_MODULE_STD)

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@apolukhin apolukhin Apr 11, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

target_compile_definitions(boost_pfr PUBLIC BOOST_USE_STD_MODULE)
target_compile_features(boost_pfr PUBLIC cxx_std_23)
endif()

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)

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

else()
add_library(boost_pfr INTERFACE)
target_include_directories(boost_pfr INTERFACE ${CMAKE_CURRENT_LIST_DIR}/include)
endif()

add_library(Boost::pfr ALIAS boost_pfr)

enable_testing()
if (BUILD_TESTING AND EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/test/CMakeLists.txt")
add_subdirectory(test)
add_subdirectory(test)
endif()

if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.28.0" AND BOOST_USE_MODULES AND BUILD_TESTING)
add_executable(boost_pfr_module_usage modules/usage_sample.cpp)
target_link_libraries(boost_pfr_module_usage PRIVATE Boost::pfr)
add_test(NAME boost_pfr_module_usage COMMAND boost_pfr_module_usage)

# Make sure that mixing includes and imports is fine for different TU
add_executable(boost_pfr_module_usage_mu modules/usage_test_mu1.cpp modules/usage_test_mu2.cpp)
target_link_libraries(boost_pfr_module_usage_mu PRIVATE Boost::pfr)
add_test(NAME boost_pfr_module_usage_mu COMMAND boost_pfr_module_usage_mu)
endif()

14 changes: 4 additions & 10 deletions include/boost/pfr/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#define BOOST_PFR_CONFIG_HPP
#pragma once

#if __cplusplus >= 201402L || (defined(_MSC_VER) && defined(_MSVC_LANG) && _MSC_VER > 1900)
#if !defined(BOOST_USE_MODULES) && (__cplusplus >= 201402L || (defined(_MSC_VER) && defined(_MSVC_LANG) && _MSC_VER > 1900))
#include <type_traits> // to get non standard platform macro definitions (__GLIBCXX__ for example)
#endif

Expand Down Expand Up @@ -70,8 +70,10 @@
#endif

#ifndef BOOST_PFR_USE_STD_MAKE_INTEGRAL_SEQUENCE
# if defined(BOOST_USE_MODULES)

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

# define BOOST_PFR_USE_STD_MAKE_INTEGRAL_SEQUENCE 1
// Assume that libstdc++ since GCC-7.3 does not have linear instantiation depth in std::make_integral_sequence
# if defined( __GLIBCXX__) && __GLIBCXX__ >= 20180101
# elif defined( __GLIBCXX__) && __GLIBCXX__ >= 20180101
# define BOOST_PFR_USE_STD_MAKE_INTEGRAL_SEQUENCE 1
# elif defined(_MSC_VER)
# define BOOST_PFR_USE_STD_MAKE_INTEGRAL_SEQUENCE 1
Expand Down Expand Up @@ -145,12 +147,4 @@

#undef BOOST_PFR_NOT_SUPPORTED

#ifndef BOOST_PFR_BEGIN_MODULE_EXPORT
# define BOOST_PFR_BEGIN_MODULE_EXPORT
#endif

#ifndef BOOST_PFR_END_MODULE_EXPORT
# define BOOST_PFR_END_MODULE_EXPORT
#endif

#endif // BOOST_PFR_CONFIG_HPP
18 changes: 12 additions & 6 deletions include/boost/pfr/core.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@

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

#if defined(BOOST_USE_MODULES) && !defined(BOOST_PFR_INTERFACE_UNIT)
import boost.pfr;
#else

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

#include <boost/pfr/detail/sequence_tuple.hpp>
Expand All @@ -17,10 +21,14 @@
#include <boost/pfr/detail/make_integer_sequence.hpp>
#include <boost/pfr/detail/tie_from_structure_tuple.hpp>

#include <boost/pfr/tuple_size.hpp>

#if defined(BOOST_USE_STD_MODULE)

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

import std;
#else

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

@apolukhin apolukhin Apr 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

#include <type_traits>
#include <utility> // metaprogramming stuff

#include <boost/pfr/tuple_size.hpp>
#endif

/// \file boost/pfr/core.hpp
/// Contains all the basic tuple-like interfaces \forcedlink{get}, \forcedlink{tuple_size}, \forcedlink{tuple_element_t}, and others.
Expand All @@ -29,8 +37,6 @@

namespace boost { namespace pfr {

BOOST_PFR_BEGIN_MODULE_EXPORT

/// \brief Returns reference or const reference to a field with index `I` in \aggregate `val`.
/// Overload taking the type `U` returns reference or const reference to a field
/// with provided type `U` in \aggregate `val` if there's only one field of such type in `val`.
Expand Down Expand Up @@ -245,8 +251,8 @@ constexpr detail::tie_from_structure_tuple<Elements...> tie_from_structure(Eleme
return detail::tie_from_structure_tuple<Elements...>(args...);
}

BOOST_PFR_END_MODULE_EXPORT

}} // namespace boost::pfr

#endif // #if defined(BOOST_USE_MODULES) && !defined(BOOST_PFR_INTERFACE_UNIT)

#endif // BOOST_PFR_CORE_HPP
18 changes: 12 additions & 6 deletions include/boost/pfr/core_name.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,24 @@

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

#if defined(BOOST_USE_MODULES) && !defined(BOOST_PFR_INTERFACE_UNIT)
import boost.pfr;
#else

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

#include <boost/pfr/detail/sequence_tuple.hpp>
#include <boost/pfr/detail/stdarray.hpp>
#include <boost/pfr/detail/make_integer_sequence.hpp>

#include <cstddef> // for std::size_t

#include <boost/pfr/tuple_size.hpp>

#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <cstddef> // for std::size_t
#endif

/// \file boost/pfr/core_name.hpp
/// Contains functions \forcedlink{get_name} and \forcedlink{names_as_array} to know which names each field of any \aggregate has.
///
Expand All @@ -33,8 +41,6 @@

namespace boost { namespace pfr {

BOOST_PFR_BEGIN_MODULE_EXPORT

/// \brief Returns name of a field with index `I` in \aggregate `T`.
///
/// \b Example:
Expand Down Expand Up @@ -106,8 +112,8 @@ constexpr void for_each_field_with_name(T&& value, F&& func) {
return boost::pfr::detail::for_each_field_with_name(std::forward<T>(value), std::forward<F>(func));
}

BOOST_PFR_END_MODULE_EXPORT

}} // namespace boost::pfr

#endif // #if defined(BOOST_USE_MODULES) && !defined(BOOST_PFR_INTERFACE_UNIT)

#endif // BOOST_PFR_CORE_NAME_HPP
14 changes: 7 additions & 7 deletions include/boost/pfr/detail/core14_classic.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,6 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
import std;
#else
#include <type_traits>
#include <utility> // metaprogramming stuff
#endif

#include <boost/pfr/detail/sequence_tuple.hpp>
#include <boost/pfr/detail/offset_based_getter.hpp>
#include <boost/pfr/detail/fields_count.hpp>
Expand All @@ -25,6 +18,13 @@ import std;
#include <boost/pfr/detail/size_t_.hpp>
#include <boost/pfr/detail/rvalue_t.hpp>

#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <type_traits>
#include <utility> // metaprogramming stuff
#endif

#ifdef __clang__
# pragma clang diagnostic push
# pragma clang diagnostic ignored "-Wmissing-braces"
Expand Down
2 changes: 1 addition & 1 deletion include/boost/pfr/detail/core14_loophole.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <type_traits>
Expand Down
2 changes: 1 addition & 1 deletion include/boost/pfr/detail/core17_generated.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
#include <boost/pfr/detail/sequence_tuple.hpp>
#include <boost/pfr/detail/size_t_.hpp>

#ifdef BOOST_PFR_HAS_STD_MODULE
#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <type_traits> // for std::conditional_t, std::is_reference
Expand Down
2 changes: 1 addition & 1 deletion include/boost/pfr/detail/core_name20_static.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
#include <boost/pfr/detail/sequence_tuple.hpp>
#include <boost/pfr/detail/stdarray.hpp>

#ifdef BOOST_PFR_HAS_STD_MODULE
#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <type_traits>
Expand Down
2 changes: 1 addition & 1 deletion include/boost/pfr/detail/detectors.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <functional>
Expand Down
2 changes: 1 addition & 1 deletion include/boost/pfr/detail/fields_count.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include <boost/pfr/detail/size_t_.hpp>
#include <boost/pfr/detail/unsafe_declval.hpp>

#ifdef BOOST_PFR_HAS_STD_MODULE
#if defined(BOOST_USE_STD_MODULE)

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

import std;
#else
#include <climits> // CHAR_BIT

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@apolukhin apolukhin Apr 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Expand Down
12 changes: 6 additions & 6 deletions include/boost/pfr/detail/for_each_field.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
import std;
#else
#include <type_traits> // metaprogramming stuff
#endif

#include <boost/pfr/detail/core.hpp>
#include <boost/pfr/detail/fields_count.hpp>
#include <boost/pfr/detail/for_each_field_impl.hpp>
#include <boost/pfr/detail/make_integer_sequence.hpp>

#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <type_traits> // metaprogramming stuff
#endif

namespace boost { namespace pfr { namespace detail {

template <class T, class F>
Expand Down
8 changes: 4 additions & 4 deletions include/boost/pfr/detail/for_each_field_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
#include <boost/pfr/detail/sequence_tuple.hpp>
#include <boost/pfr/detail/rvalue_t.hpp>

#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <utility> // metaprogramming stuff
#endif

#include <boost/pfr/detail/sequence_tuple.hpp>
#include <boost/pfr/detail/rvalue_t.hpp>

namespace boost { namespace pfr { namespace detail {

template <std::size_t Index>
Expand Down
6 changes: 3 additions & 3 deletions include/boost/pfr/detail/functional.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
#include <boost/pfr/detail/sequence_tuple.hpp>

#if defined(BOOST_USE_STD_MODULE)

@anarthal anarthal Apr 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same with INT64_MIN and friends here.

import std;
#else
#include <functional>
#include <cstdint>
#endif

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

namespace boost { namespace pfr { namespace detail {
template <std::size_t I, std::size_t N>
struct equal_impl {
Expand Down
2 changes: 1 addition & 1 deletion include/boost/pfr/detail/io.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <iosfwd> // stream operators
Expand Down
11 changes: 5 additions & 6 deletions include/boost/pfr/detail/make_flat_tuple_of_references.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,15 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
import std;
#else
#include <utility> // metaprogramming stuff
#endif

#include <boost/pfr/detail/sequence_tuple.hpp>
#include <boost/pfr/detail/rvalue_t.hpp>
#include <boost/pfr/detail/make_integer_sequence.hpp>

#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <utility> // metaprogramming stuff
#endif

namespace boost { namespace pfr { namespace detail {

Expand Down
2 changes: 1 addition & 1 deletion include/boost/pfr/detail/make_integer_sequence.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

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

#ifdef BOOST_PFR_HAS_STD_MODULE
#if defined(BOOST_USE_STD_MODULE)
import std;
#else
#include <type_traits>
Expand Down
Loading