Implement C++20 module system with MPI support#90
Conversation
- Configure Ninja generator for C++ module builds - Add gcc-15/llvm-21 toolchains in CI - Stabilize CI dependencies and pin CMake 4.0.2 - Update CMake gates for version 4.2 - Harden CI gcc-15 installation and module links - Fix header file set install for interface targets - Build libc++ modules on macOS when missing - Align CI toolchain and CMake installs - Make std-module patch resilient and non-fatal Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Fix module export leakage to consumers - Fix INTERFACE target wiring for libc++ module detection - Add fallback path for libc++ modules.json - Trim amgcl module and fix MPI communication - Fix module exports for clang and GCC - Adjust ext.amgcl module for GCC compatibility - Mute thread-safety warnings in modules - Propagate MPI definitions to module builds Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Provide dummy MPI types when MPI is disabled - Fix MPI logging macro guards - Guard dummy MPI typedef when MPI headers present - Include mpi.h when available in non-MPI builds - Disable MPI C++ bindings globally - Initialize MPI when headers are available - Expose MPI init in module builds when headers exist - Guard -mfma flag for non-x86 architectures - Add macOS SDK sysroot for Homebrew clang - Fix macOS libc++ modules.json detection - Restore macOS gcc matrix and fix boost cstdint includes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Replace std::print with std::format for portability - Avoid chrono time zone APIs (require C++20 timezone support) - Fix amgcl test includes and std::print usage - Update C++ headers to use standard library consistently - Apply code formatting across the codebase Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 221 files, which is 71 over the limit of 150. You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Luohaothu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the project's build infrastructure and C++ standard adoption by integrating a comprehensive C++20 module system. It introduces experimental module builds, refines MPI support for better compatibility and robustness, and includes several platform-specific fixes, particularly for macOS. The changes aim to modernize the codebase, improve build times, and ensure broader compiler and environment compatibility, while maintaining support for traditional header-only builds. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2dbc728de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void inline InitEnvironment(int* argc, char*** argv) { | ||
| #ifdef OPFLOW_WITH_MPI | ||
| #if defined(OPFLOW_WITH_MPI) || defined(OPFLOW_HAS_MPI) | ||
| MPI_Init(argc, argv); | ||
| #endif |
There was a problem hiding this comment.
Gate MPI_Init/Finalize on OPFLOW_WITH_MPI only
When mpi.h is present, OPFLOW_HAS_MPI becomes defined and InitEnvironment now calls MPI_Init even if OPFLOW_WITH_MPI is OFF. On systems that have MPI headers installed but aren’t linking the MPI libraries (the normal case when the option is OFF), this introduces undefined references at link time and changes runtime behavior by initializing MPI unexpectedly. Please keep the MPI calls guarded by the build option rather than header availability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive C++20 module support and enhances MPI integration, which is a significant and well-executed update. The changes to the CMake build system are robust, correctly handling the differences between header-only and module-based builds. I appreciate the numerous portability improvements, such as replacing non-standard std::format usage, making compiler flags architecture-specific, and providing dummy MPI types for non-MPI builds. The code quality has also been improved by removing redundant keywords and correcting the usage of inline with static. Overall, this is an excellent contribution. I have one minor suggestion for code cleanup in a CMake file.
| if(OPFLOW_ENABLE_MODULE) | ||
| set(OPFLOW_TARGET_SCOPE PUBLIC) | ||
| else() | ||
| set(OPFLOW_TARGET_SCOPE INTERFACE) | ||
| endif() |
There was a problem hiding this comment.
This block of code, which sets the OPFLOW_TARGET_SCOPE variable, is a duplication of the logic already present in the root CMakeLists.txt (lines 137-141). Since CMake variables set in a parent scope are inherited by subdirectories, this block is redundant and can be removed to improve maintainability.
Summary
Add complete C++20 module support with experimental module builds and MPI integration.
Changes
Build System
Module Implementation
MPI & Platform Support
Test Plan
cmake -DOPFLOW_ENABLE_MODULE=ONcmake -DOPFLOW_WITH_MPI=ONDependencies
None - can be merged independently
🤖 Generated with Claude Code