Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds module support to the beman.execution library by introducing the BEMAN_HAS_MODULES preprocessor macro and updating test files, examples, and build configuration to support both traditional header-based and C++20 module-based compilation.
Changes:
- Added BEMAN_HAS_MODULES conditional compilation support across all test files and examples
- Updated module implementation file (execution.cppm) with export keywords and structural changes to support modules
- Modified build system (CMakeLists.txt, Makefile) to support module builds
- Refactored several header files to properly export symbols for module support
- Fixed issues in join_env and default_impls to support module compilation
Reviewed changes
Copilot reviewed 120 out of 120 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/beman/execution/*.test.cpp | Added BEMAN_HAS_MODULES guards to conditionally import module or include headers |
| tests/beman/execution/include/test/*.hpp | Updated test utility headers with module support |
| src/beman/execution/execution.cppm | Added export keywords and refactored code structure for module support |
| include/beman/execution/detail/*.hpp | Added BEMAN_EXECUTION_EXPORT macros to support module compilation |
| examples/*.cpp | Added module support with BEMAN_HAS_MODULES conditional compilation |
| tests/beman/execution/CMakeLists.txt | Added BEMAN_HAS_MODULES definition for module builds and reorganized test list |
| examples/CMakeLists.txt | Added module build infrastructure |
| Makefile | Added module and build-module targets for building with modules enabled |
| infra/cmake/llvm-toolchain.cmake | Removed -fsanitize=leak from MaxSan configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <utility> | ||
| #include <tuple> | ||
| #include <variant> | ||
| // #include <beman/execution/detail/suppress_push.hpp> |
There was a problem hiding this comment.
The commented-out include for suppress_push.hpp should either be uncommented if it's needed or removed if it's not. Leaving commented code creates confusion about whether this include is actually required.
| #if false && defined(BEMAN_HAS_MODULES) | ||
| import beman.execution; | ||
| #else | ||
| #include <beman/execution/stop_token.hpp> | ||
| #endif |
There was a problem hiding this comment.
The module import is disabled with #if false. Several test files have this same pattern where modules are deliberately disabled. This creates inconsistency in testing - some tests use modules when BEMAN_HAS_MODULES is defined, while others explicitly disable it. Consider either enabling module support consistently or documenting why certain tests cannot use modules.
| #if false && defined(BEMAN_HAS_MODULES) | ||
| import beman.execution; | ||
| #else | ||
| #include <beman/execution/stop_token.hpp> | ||
| #endif |
There was a problem hiding this comment.
The module import is disabled with #if false. This appears to be intentional (possibly a workaround), but it means the module path is never tested for these files. This inconsistency across test files should be documented or the disabled code should be removed if modules are not yet supported for these specific test cases.
2 participants
No description provided.