Initial C++20 module implementation #72
Conversation
📝 WalkthroughWalkthroughThis pull request adds C++20 module support to Boost Corosio. A new CMake option controls whether to build with modules enabled. A module interface file exports the public API, and header files conditionally import the module instead of including headers when modules are enabled. Changes
Sequence DiagramsequenceDiagram
actor Dev as Developer
participant CMake as CMake Build<br/>System
participant Headers as Header Files
participant Module as Module Interface<br/>(boost_corosio.cppm)
participant Compiler as C++ Compiler
Dev->>CMake: Configure with BOOST_COROSIO_USE_MODULES=ON
CMake->>CMake: Enable CXX_MODULES for boost_corosio target
CMake->>Compiler: Pass module build configuration
Dev->>Compiler: Compile with module build
Compiler->>Headers: Check conditional guards
alt Module Build Enabled
Headers->>Module: import boost.corosio
Module->>Module: Export public API symbols<br/>(types, functions, TLS namespace)
Module-->>Compiler: Module unit compiled & cached
else Traditional Build
Headers->>Headers: `#include` traditional headers
Headers-->>Compiler: Process headers normally
end
Compiler-->>Dev: Translation unit compiled
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 189-192: The CMake snippet uses FILE_SET CXX_MODULES which
requires CMake 3.28+, so add a CMake version guard around the
BOOST_COROSIO_USE_MODULES branch: check CMAKE_VERSION and if it is less than
3.28 produce a clear fatal error or disable modules with a descriptive message
referencing BOOST_COROSIO_USE_MODULES, otherwise proceed to call
target_sources(boost_corosio PUBLIC FILE_SET CXX_MODULES BASE_DIRS modules FILES
modules/boost_corosio.cppm) and target_compile_definitions(boost_corosio PUBLIC
BOOST_COROSIO_USE_MODULES); ensure the error message and guard mention CMake
3.28+ and the option name so users get a clear instruction.
In `@modules/boost_corosio.cppm`:
- Around line 30-33: Remove the non-actionable inline comment "// I think this
should be in tls" above the using declarations for tls_stream, openssl_stream,
and wolfssl_stream; either delete it or replace it with a concrete note/TODO
explaining why these aliases belong in a separate tls module (e.g., "TODO: move
these transport stream aliases to corosio::tls to centralize TLS abstractions")
so reviewers have a clear, actionable rationale referencing tls_stream,
openssl_stream, and wolfssl_stream.
- Around line 1-4: The module interface unit is missing a local definition of
BOOST_COROSIO_SOURCE so headers guarded by BOOST_COROSIO_USE_MODULES try to
import boost.corosio while the module is being defined; add a local `#define`
BOOST_COROSIO_SOURCE at the top of the module file (before the `#include`
<boost/corosio.hpp>) so the headers see the source definition and use includes
instead of emitting import boost.corosio, preventing the self-import/circular
dependency.
🧹 Nitpick comments (1)
include/boost/corosio/tls/wolfssl_stream.hpp (1)
21-31: Missing high-level overview comment.Per coding guidelines,
.hppfiles should have a/* */section after the includes that gives a high-level overview of how the implementation works. Consider adding a brief description of the WolfSSL TLS stream wrapper's purpose and design.
| if (BOOST_COROSIO_USE_MODULES) | ||
| target_sources(boost_corosio PUBLIC FILE_SET CXX_MODULES BASE_DIRS modules FILES modules/boost_corosio.cppm) | ||
| target_compile_definitions(boost_corosio PUBLIC BOOST_COROSIO_USE_MODULES) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CMake FILE_SET CXX_MODULES minimum version requirement
💡 Result:
Official (non-experimental) minimum CMake version for FILE_SET TYPE CXX_MODULES: 3.28 — the CXX_MODULES file-set type for target_sources(FILE_SET ...) was added in CMake 3.28. (cmake.org)
Earlier versions: CMake had experimental C++ modules support (gated by CMAKE_EXPERIMENTAL_CXX_MODULE_CMAKE_API) before 3.28, but the stable FILE_SET TYPE CXX_MODULES feature is 3.28+. (kitware.com)
Citations:
- 1: https://cmake.org/cmake/help/v3.31/command/target_sources.html?utm_source=openai
- 2: https://www.kitware.com/import-cmake-c20-modules/?utm_source=openai
🏁 Script executed:
# Check the CMakeLists.txt file for minimum CMake version and the problematic code
head -20 CMakeLists.txt | cat -nRepository: cppalliance/corosio
Length of output: 809
🏁 Script executed:
# Check lines 189-192 in CMakeLists.txt
sed -n '189,192p' CMakeLists.txtRepository: cppalliance/corosio
Length of output: 293
🏁 Script executed:
# Also verify surrounding context to understand the structure
sed -n '16p' CMakeLists.txtRepository: cppalliance/corosio
Length of output: 106
Add CMake version guard for C++20 modules support.
FILE_SET CXX_MODULES requires CMake 3.28+, but the project's minimum version is 3.16 (line 16). If a user enables BOOST_COROSIO_USE_MODULES with CMake < 3.28, they'll get a confusing error.
Proposed fix: add version check
if (BOOST_COROSIO_USE_MODULES)
+ if (CMAKE_VERSION VERSION_LESS "3.28")
+ message(FATAL_ERROR "BOOST_COROSIO_USE_MODULES requires CMake 3.28 or later")
+ endif()
target_sources(boost_corosio PUBLIC FILE_SET CXX_MODULES BASE_DIRS modules FILES modules/boost_corosio.cppm)
target_compile_definitions(boost_corosio PUBLIC BOOST_COROSIO_USE_MODULES)
endif()🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 189 - 192, The CMake snippet uses FILE_SET
CXX_MODULES which requires CMake 3.28+, so add a CMake version guard around the
BOOST_COROSIO_USE_MODULES branch: check CMAKE_VERSION and if it is less than
3.28 produce a clear fatal error or disable modules with a descriptive message
referencing BOOST_COROSIO_USE_MODULES, otherwise proceed to call
target_sources(boost_corosio PUBLIC FILE_SET CXX_MODULES BASE_DIRS modules FILES
modules/boost_corosio.cppm) and target_compile_definitions(boost_corosio PUBLIC
BOOST_COROSIO_USE_MODULES); ensure the error message and guard mention CMake
3.28+ and the option name so users get a clear instruction.
| module; | ||
|
|
||
| #include <boost/corosio.hpp> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | head -20Repository: cppalliance/corosio
Length of output: 421
🏁 Script executed:
# Find key files related to this review
fd -t f "corosio\.cppm|CMakeLists\.txt" | head -20Repository: cppalliance/corosio
Length of output: 336
🏁 Script executed:
# Look for the module interface file and main header
find . -name "*boost_corosio*" -o -name "corosio.hpp" | grep -E "\.(cpp|cppm|h|hpp)$"Repository: cppalliance/corosio
Length of output: 119
🏁 Script executed:
# Check the actual module interface file mentioned in the review
cat -n modules/boost_corosio.cppm 2>/dev/null || echo "File not found, searching for it..."Repository: cppalliance/corosio
Length of output: 1450
🏁 Script executed:
# Search for include/boost/corosio structure
fd -t f . include/boost/corosio 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 829
🏁 Script executed:
# Read the main header file
cat -n include/boost/corosio.hpp | head -50Repository: cppalliance/corosio
Length of output: 1276
🏁 Script executed:
# Read the config header which likely defines the macros
cat -n include/boost/corosio/detail/config.hppRepository: cppalliance/corosio
Length of output: 1651
🏁 Script executed:
# Search for BOOST_COROSIO_USE_MODULES and BOOST_COROSIO_SOURCE usage in headers
rg -n "BOOST_COROSIO_USE_MODULES|BOOST_COROSIO_SOURCE" include/Repository: cppalliance/corosio
Length of output: 2330
🏁 Script executed:
# Check CMake configuration for the module unit
cat -n CMakeLists.txt | head -80Repository: cppalliance/corosio
Length of output: 3406
🏁 Script executed:
# Look for how modules/boost_corosio.cppm is built in CMake
rg -n "boost_corosio\.cppm|corosio.*INTERFACE_UNITS|CXX_MODULE" CMakeLists.txt test/CMakeLists.txtRepository: cppalliance/corosio
Length of output: 194
🏁 Script executed:
# Check what happens in one of the headers when the condition is true
cat -n include/boost/corosio/socket.hpp | head -40Repository: cppalliance/corosio
Length of output: 1449
🏁 Script executed:
# Search for "import boost.corosio" in headers to confirm the recursive import concern
rg -n "import\s+boost\.corosio" include/Repository: cppalliance/corosio
Length of output: 1189
🏁 Script executed:
# Check the CMakeLists around line 190 for context on how BOOST_COROSIO_SOURCE is defined for the module
sed -n '180,200p' CMakeLists.txtRepository: cppalliance/corosio
Length of output: 860
🏁 Script executed:
# Look for target_compile_definitions for boost_corosio related to BOOST_COROSIO_SOURCE
rg -n "target_compile_definitions.*boost_corosio|BOOST_COROSIO_SOURCE" CMakeLists.txtRepository: cppalliance/corosio
Length of output: 743
🏁 Script executed:
# Find the boost_corosio_setup_properties function to see if it applies BOOST_COROSIO_SOURCE
rg -n "boost_corosio_setup_properties" CMakeLists.txt -A 20Repository: cppalliance/corosio
Length of output: 4487
🏁 Script executed:
# Get more context around line 159 where BOOST_COROSIO_SOURCE is defined
sed -n '150,170p' CMakeLists.txtRepository: cppalliance/corosio
Length of output: 843
🏁 Script executed:
# Look for any place where BOOST_COROSIO_SOURCE is set for the module interface
rg -n "modules/boost_corosio\.cppm\|FILE_SET.*CXX_MODULES" CMakeLists.txt -B 5 -A 10Repository: cppalliance/corosio
Length of output: 45
Add BOOST_COROSIO_SOURCE define to the module interface unit to prevent self-import.
When BOOST_COROSIO_USE_MODULES is enabled, all headers check #if !defined(BOOST_COROSIO_SOURCE) && defined(BOOST_COROSIO_USE_MODULES) and emit import boost.corosio; instead of includes. Since BOOST_COROSIO_SOURCE is defined as PRIVATE in the CMake target but the module interface unit may not reliably receive private definitions, the headers will attempt to import the module while it's being defined, causing a circular dependency. Define it locally in the module file:
Suggested fix
module;
+#define BOOST_COROSIO_SOURCE 1
`#include` <boost/corosio.hpp>
+#undef BOOST_COROSIO_SOURCE📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module; | |
| #include <boost/corosio.hpp> | |
| module; | |
| `#define` BOOST_COROSIO_SOURCE 1 | |
| `#include` <boost/corosio.hpp> | |
| `#undef` BOOST_COROSIO_SOURCE | |
🤖 Prompt for AI Agents
In `@modules/boost_corosio.cppm` around lines 1 - 4, The module interface unit is
missing a local definition of BOOST_COROSIO_SOURCE so headers guarded by
BOOST_COROSIO_USE_MODULES try to import boost.corosio while the module is being
defined; add a local `#define` BOOST_COROSIO_SOURCE at the top of the module file
(before the `#include` <boost/corosio.hpp>) so the headers see the source
definition and use includes instead of emitting import boost.corosio, preventing
the self-import/circular dependency.
| // I think this should be in tls | ||
| using corosio::tls_stream; | ||
| using corosio::openssl_stream; | ||
| using corosio::wolfssl_stream; |
There was a problem hiding this comment.
Remove or clarify the inline note.
The // I think this should be in tls comment isn’t actionable and doesn’t explain the “why”; please remove it or replace with a concrete rationale/TODO. As per coding guidelines, ...
🧹 Suggested cleanup
-// I think this should be in tls
-
using corosio::tls_stream;
using corosio::openssl_stream;
using corosio::wolfssl_stream;🤖 Prompt for AI Agents
In `@modules/boost_corosio.cppm` around lines 30 - 33, Remove the non-actionable
inline comment "// I think this should be in tls" above the using declarations
for tls_stream, openssl_stream, and wolfssl_stream; either delete it or replace
it with a concrete note/TODO explaining why these aliases belong in a separate
tls module (e.g., "TODO: move these transport stream aliases to corosio::tls to
centralize TLS abstractions") so reviewers have a clear, actionable rationale
referencing tls_stream, openssl_stream, and wolfssl_stream.
2 participants
Closes #70.
Resolves #26.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.