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>
Update copyright notices in source files from 2025 to 2026. 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>
📝 WalkthroughWalkthroughThis pull request significantly expands the build infrastructure, adds C++ module support throughout the codebase, and performs comprehensive code hygiene improvements. Changes include a matrix-based GitHub Actions workflow supporting multiple OS/compiler combinations, extensive parameter naming/type safety refinements, virtual destructor additions to interfaces, conditional module imports in tests, and MPI integration adjustments. Changes
Sequence Diagram(s)Sequence diagrams are not applicable for this pull request. The changes are primarily build/infrastructure updates, code hygiene improvements, module system integration, and API refinements without introducing new feature flows or significantly altering component interactions requiring visualization. Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 focuses on improving the core compilation process, updating copyright information, enhancing MPI compatibility, and ensuring unique output directories for example runs. The changes contribute to a more robust and maintainable codebase. Highlights
Ignored Files
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a series of changes to fix module compilation issues and improve overall build system compatibility and robustness. Key changes include:
- Conditional CMake logic for experimental features based on CMake version.
- Correct handling of architecture-specific compiler flags.
- Replacement of C++23
std::printand C++20 chrono formatting instd::formatwith more widely supported alternatives (std::coutandstrftime), enhancing compiler compatibility. - Refactoring of CMake scripts to use a consistent
OPFLOW_TARGET_SCOPEvariable, improving maintainability. - Conditional includes in test files to support both module and header-only builds.
- Fixes for MPI C++ bindings and vendored library
amgcl.
The changes are well-thought-out and address the goal of fixing compilation issues effectively. I have one minor suggestion to improve code clarity in the CMake scripts by removing a redundant variable definition.
| if(OPFLOW_ENABLE_MODULE) | ||
| set(OPFLOW_TARGET_SCOPE PUBLIC) | ||
| else() | ||
| set(OPFLOW_TARGET_SCOPE INTERFACE) | ||
| endif() |
There was a problem hiding this comment.
This if/else block to set OPFLOW_TARGET_SCOPE is redundant. The same logic is already present in the root CMakeLists.txt file, and the variable is set before this script is included. Removing this redundant block will make the build configuration cleaner and easier to maintain.
…lized In Release mode with -O3 optimization, GCC 15 emits -Werror=maybe-uninitialized for the ret variable in split_impl because it cannot prove all elements are initialized before use. Fix by initializing ret with _range. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Clang with -Werror,-Wunused-but-set-variable catches that variable i is declared but never used in the for loop at distributed_matrix.hpp:691. Remove the unused variable to fix clang/MPION builds. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Clang with -Werror,-Wsign-compare catches comparisons between signed and unsigned integers. Fix by adding casts or using size_t: - CartesianField.hpp: Cast .size() to int for comparison - TecplotBinaryStream.hpp: Use size_t for loop variable - TecplotSZPLTStream.hpp: Use size_t for loop variable Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Clang emits -Wdelete-non-abstract-non-virtual-dtor when classes with virtual functions lack virtual destructors. Add virtual destructors to: - AbstractSplitStrategy<F> (base class) - EvenSplitStrategy<F> (derived class) This ensures proper cleanup when deleting through base class pointer. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add virtual destructor to SerializableObj base class - Add final keyword to Meta::int_ and Meta::bool_ types - Add virtual destructor to Int test class in CartesianFieldMPITest These changes fix the clang/libc++ warnings about classes with virtual functions but non-virtual destructors that become errors with -Werror.
|
@claude review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb6b190d99
ℹ️ 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".
| echo "$(brew --prefix)/bin" >> "$GITHUB_PATH" | ||
| brew install doxygen tbb lcov boost open-mpi ninja sphinx-doc | ||
| echo "$(brew --prefix sphinx-doc)/bin" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
The macOS branch installs LLVM but not libomp (brew install doxygen tbb lcov boost open-mpi ninja sphinx-doc), while CMakeLists.txt enables OpenMP by default (OPFLOW_WITH_OPENMP is ON and find_package(OpenMP REQUIRED) is called). On macOS/clang (now in the matrix), OpenMP comes from libomp, so CMake will fail to find OpenMP and the job will error. Consider adding brew install libomp (or turning off OpenMP for macOS clang) to keep these builds passing.
Useful? React with 👍 / 👎.
Incorporate PR #92 changes (disable module build to fix CI) while preserving fix-module branch's comprehensive module support fixes. Conflict resolutions: - Build.yml: Keep fix-module's complete CI with gcc-15/clang-21 toolchains and conditional module support (PR #92's was temporary) - CMakeLists.txt: Keep fix-module's experimental CMake settings at file top; auto-merged OPFLOW_TARGET_SCOPE from PR #92 - Test files: Keep fix-module's #ifdef OPFLOW_USE_MODULE conditional imports instead of PR #92's hardcoded #include <OpFlow> - test_main*.cpp: Fixed duplicate #include <OpFlow> from auto-merge Auto-merged from PR #92: - OPFLOW_TARGET_SCOPE (PUBLIC/INTERFACE based on module mode) - MPI --allow-run-as-root flag for CI containers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/DataStructures/Index/LinearMapper/GeneralBlockedMDRangeMapper.hpp (1)
76-86:⚠️ Potential issue | 🟡 MinorPotential narrowing conversion and remaining signed/unsigned comparison.
- Line 76 returns
i(nowstd::size_t) asint, which can cause narrowing if there are more thanINT_MAXranges.- Line 78 assigns
std::size_t itolast_block_rank(typeint), which is another narrowing conversion.- Line 54 (unchanged) still uses
int icomparing against_ranges.size(), which will produce signed/unsigned comparison warnings.While unlikely to cause issues in practice given typical range counts, these inconsistencies undermine the warning-fix goal of this change.
Suggested fix for consistency
- int getBlockRank(const MDIndex<d>& idx) const { + std::size_t getBlockRank(const MDIndex<d>& idx) const { if (!checkIndexInBlock(idx, last_block_rank)) { // loop over all ranges for (std::size_t i = 0; i < _ranges.size(); ++i) {And update
last_block_ranktostd::size_tas well, along withcheckIndexInBlock's second parameter. Alternatively, usestatic_cast<int>(i)at the return sites ifintreturn type must be preserved for API compatibility.src/Core/Field/MeshBased/Structured/CartesianField.hpp (1)
794-806:⚠️ Potential issue | 🟠 MajorWritable-range check should use
getLocalWritableRange().
The non-const accessor currently asserts against the readable range, which can allow writes into non-writable halo regions. That’s a correctness risk when mutating the field.🛠️ Suggested fix
- OP_ASSERT_MSG(DS::inRange(this->getLocalReadableRange(), i), + OP_ASSERT_MSG(DS::inRange(this->getLocalWritableRange(), i), "Cannot eval {} at {}: out of range {}", this->getName(), i, this->getLocalWritableRange().toString());src/Core/Equation/HYPREEqnSolveHandler.hpp (1)
63-69:⚠️ Potential issue | 🔴 CriticalGuard HYPRE destroys in the destructor.
The destructor calls
HYPRE_*Destroyon handles that may be uninitialized if a default-constructed handler is never initialized viainit(). The HYPRE handles are zero-initialized (lines 223-226), and destroying invalid handles can cause a crash. Use thefieldsAllocatedflag to guard the destroy calls, as already done in the second template specialization'sdeallocHYPRE()method (line 317).🛠️ Suggested fix
~HYPREEqnSolveHandler() override { + if (!fieldsAllocated) return; HYPRE_StructMatrixDestroy(A); HYPRE_StructVectorDestroy(b); HYPRE_StructVectorDestroy(x); HYPRE_StructGridDestroy(grid); HYPRE_StructStencilDestroy(stencil); }src/Core/Macros.hpp (2)
72-80:⚠️ Potential issue | 🔴 CriticalIncorrect use of
if consteval- this will never execute the logging code at runtime.The
if constevalblock only executes when the enclosing function is being evaluated at compile time. Since logging is inherently a runtime operation, this code will never execute thespdlog::X(...)call at runtime. The logging statements will be silently skipped.This appears to be a misunderstanding of
if consteval. If the intent was to useif constexprfor some compile-time check, that would require a constant expression condition. If runtime logging is desired, remove theif constevalwrapper entirely.🐛 Proposed fix to enable runtime logging
`#define` SPD_AUGMENTED_LOG(X, ...) \ do { \ - if \ - consteval { \ - spdlog::X(std::format("[{}:{}@{}][Rank{}] ", __FILENAME__, __FUNCTION__, __LINE__, \ - OpFlow::getWorkerId(MPI_COMM_WORLD)) \ - + std::format(__VA_ARGS__)); \ - } \ + spdlog::X(std::format("[{}:{}@{}][Rank{}] ", __FILENAME__, __FUNCTION__, __LINE__, \ + OpFlow::getWorkerId(MPI_COMM_WORLD)) \ + + std::format(__VA_ARGS__)); \ } while (0)
94-101:⚠️ Potential issue | 🔴 CriticalSame
if constevalissue in non-MPI logging path.This suffers from the same problem as the MPI variant - logging will never execute at runtime.
🐛 Proposed fix
`#define` SPD_AUGMENTED_LOG(X, ...) \ do { \ - if \ - consteval { \ - spdlog::X(std::format("[{}:{}@{}] ", __FILENAME__, __FUNCTION__, __LINE__) \ - + std::format(__VA_ARGS__)); \ - } \ + spdlog::X(std::format("[{}:{}@{}] ", __FILENAME__, __FUNCTION__, __LINE__) \ + + std::format(__VA_ARGS__)); \ } while (0)
🤖 Fix all issues with AI agents
In @.github/workflows/Build.yml:
- Around line 42-47: Replace the mutable reference to the checkout action so it
uses a stable pinned release: update the GitHub Actions step that sets "uses:
actions/checkout@master" to use the stable tag "actions/checkout@v6" (leave
existing step keys like "name: Checkout", "with: submodules: recursive", and
"fetch-depth: 0" unchanged).
In `@CMakeLists.txt`:
- Around line 18-29: The CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE value for
pre-4.2 CMake may be incorrect; open the CMake dev documentation
(Help/dev/experimental.rst) corresponding to the pre-4.2 target and verify the
correct GUID, then update the CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE variable
(the quoted value "4bd552e2-b7fb-429a-ab23-c83ef53f3f13") in the CMakeLists to
the verified GUID (or add a clarifying comment linking to the exact dev
doc/version if multiple values exist) so configure won't fail on older CMake
versions.
In `@examples/LevelSet/UniLS.cpp`:
- Around line 203-204: The variable root (result of make_result_root()) is
unused while the Tecplot files are written to hardcoded names; either remove the
unused root declaration or use it to prefix the output filenames. If choosing to
use root, update the Utils::TecplotASCIIStream constructions (uf, vf, wf, pf) to
build paths by concatenating root with "u.tec", "v.tec", "w.tec", "p.tec" and
ensure the directory pointed to by root is created before constructing the
streams (e.g., call the existing directory-creation helper or
std::filesystem::create_directories on root). If removing root, simply delete
the root = make_result_root() line.
In `@src/CMakeLists.txt`:
- Around line 95-99: The OPFLOW_TARGET_SCOPE variable is defined too late and is
expanded empty when used; move the conditional block that sets
OPFLOW_TARGET_SCOPE (the if(OPFLOW_ENABLE_MODULE) ... endif() block) so it
appears before the first use of ${OPFLOW_TARGET_SCOPE} in the
target_include_directories call, ensuring subsequent uses in
target_compile_definitions and target_compile_options see a proper PUBLIC or
INTERFACE scope.
In `@src/Core/Environment.hpp`:
- Around line 16-25: The preprocessor guard currently enables OPFLOW_HAS_MPI
when either OPFLOW_WITH_MPI is defined or mpi.h exists; change the OR to AND so
OPFLOW_HAS_MPI is defined only when both OPFLOW_WITH_MPI is set and mpi.h is
present to avoid calling MPI without linkage. Update the three occurrences that
set OPFLOW_HAS_MPI (the block around the initial include and the guards
referenced in the review) so the condition reads require both OPFLOW_WITH_MPI
and __has_include(<mpi.h>), and ensure runtime calls in
InitEnvironment/FinalizeEnvironment and the auto-init via EnvironmentGardian
remain gated by OPFLOW_HAS_MPI.
In `@src/Core/Equation/HYPREEqnSolveHandler.hpp`:
- Around line 299-300: Unconditional std::cout debug prints in
HYPREEqnSolveHandler.hpp (e.g., the std::cout << std::format("GraphAddEntry: {}
-> {}\n", i, k); and the other similar prints) are executed in tight loops and
must be gated; replace these direct prints with a logging macro or debug flag
check (e.g., if (DEBUG_GRAPH) or using the project's logger) so the messages are
only emitted when verbose/debug mode is enabled, and ensure the same guard is
applied to the other occurrences referenced in this file (the GraphAddEntry and
related debug prints).
In `@src/Core/Field/MeshBased/StencilField.hpp`:
- Around line 397-399: The CartAMRFieldType specialization of the StencilField
constructor is missing a default value for the color parameter causing an API
break; update the constructor signature in the StencilField class specialization
(the explicit StencilField(const T& base, int color) for CartAMRFieldType) to
provide a default (e.g., int color = 0) so callers can construct with only the
base argument, matching the StructuredFieldExprType specialization.
In `@src/Core/Field/MeshBased/Structured/CartesianField.hpp`:
- Around line 73-75: The move constructor CartesianField(CartesianField&& other)
noexcept must reorder its member initializer list to match the class member
declaration order: ensure ext_width is initialized before initialized and bc
(and keep data where it belongs relative to declarations); update the
initializer list for CartesianFieldExpr<CartesianField<D, M,
C>>(std::move(other)), data(std::move(other.data)),
ext_width(std::move(other.ext_width)), initialized(true),
bc(std::move(other.bc)) so member initialization order matches the declaration
order and removes the -Wreorder warning.
In `@src/Core/Mesh/SemiStructured/CartesianAMRMesh.hpp`:
- Around line 69-72: The CartesianAMRMesh constructor incorrectly calls
ranges.emplace_back(baseMesh.getRange()), attempting to construct a
std::vector<DS::LevelRange<dim>> from a Range; change it to the two-stage
pattern used in MeshBuilder::setBaseMesh and
CartesianAMRMeshDirectComposer::setBaseMesh: first emplace_back an empty inner
vector into ranges, then call ranges.back().emplace_back(baseMesh.getRange()) so
that a DS::LevelRange<dim> is constructed from the Range; update the
CartesianAMRMesh(const CartesianMesh<Dim>&) constructor accordingly and keep
meshes.push_back(baseMesh) as-is.
In `@src/Core/Meta.hpp`:
- Around line 27-30: Making Meta::int_ and Meta::bool_ final is a breaking API
change because it prevents downstream inheritance; revert the "final" qualifier
on the template structs int_ and bool_ (or if intentional, add clear
deprecation/compatibility notes and a migration path) so existing code that
inherits from Meta::int_ / Meta::bool_ continues to compile; locate the
declarations of Meta::int_ and Meta::bool_ (the template struct names int_ and
bool_) and remove the final specifier, or alternatively restore previous
typedef/alias compatibility wrappers and update release notes and tests to cover
inheritance usage.
In `@src/Core/Solvers/Struct/StructSolver.hpp`:
- Around line 40-48: The base StructSolver parameter printLevel is declared as
bool but HYPRE expects an int and derived solver structs use std::optional<int>;
change the printLevel member in StructSolver.hpp from bool to std::optional<int>
(or at minimum int) so its type matches HYPRE_Struct*SetPrintLevel APIs and the
derived solver parameter structs; update any code that reads/writes
StructSolver::printLevel to handle the new int/optional<int> semantics (e.g.,
checks for has_value() or compare to 0) and ensure calls that pass printLevel to
HYPRE_StructSMGSetPrintLevel / HYPRE_StructPFMGSetPrintLevel etc. pass an int
not a bool.
In `@src/Utils/Writers/HDF5Stream.hpp`:
- Around line 198-199: The attribute [[maybe_unused]] is incorrectly applied to
the active parameter f in H5Stream::operator<< (template CartesianFieldExprType
T) — remove the attribute from the parameter so the compiler knows f is used, or
if you must silence warnings only when OPFLOW_WITH_HDF5 is not defined, wrap the
attribute in the same preprocessor guard (apply [[maybe_unused]] only in the
`#else` / non-HDF5 path) so the main implementation that calls f.prepare(),
f.getName(), f.accessibleRange, f.localRange, f.getMesh(), f.loc, f.evalAt(),
etc. remains accurate.
- Around line 300-301: The template operator H5Stream::operator>> currently
comments out the parameter name (T& /* f */) which breaks compilation because
the body references f (e.g., f.localRange, f.accessibleRange, f.getName(), f[i],
f.updatePadding()); restore the parameter name by changing the signature to
accept T& f (i.e., template <CartesianFieldType T> H5Stream&
H5Stream::operator>>(T& f)) so the function body can compile and use f as
intended.
In `@src/Utils/Writers/TecplotBinaryStream.hpp`:
- Around line 208-213: The file mixes uppercase and lowercase TecIO symbol calls
(e.g., TECIJKPTN142/TECMPIINIT142 vs tecijkptn142/tecmpiinit142) which is
inconsistent; pick one convention (prefer lowercase) and replace all occurrences
so calls like tecznemap142, tecijkptn142, tecmpiinit142 (and any other TEC*142
symbols) use the chosen casing consistently across the file (including the sites
that set partition_ranks and call tecznemap142/tecijkptn142); update every call
site so the symbol names match the chosen case exactly.
In `@test/Core/Equation/AMGCLTest.cpp`:
- Around line 15-26: Add the missing include for the AMGCL zero-copy adapter:
include the header that declares amgcl::adapter::zero_copy by adding `#include`
<amgcl/adapter/zero_copy.hpp> to the top of AMGCLTest.cpp so calls to
amgcl::adapter::zero_copy() (used later around line 55) compile; ensure it is
placed with the other amgcl includes (near amgcl/make_solver.hpp or
amgcl/amg.hpp).
In `@test/Core/Equation/EqnSetMPITest.cpp`:
- Around line 115-116: The diagnostic prints using the std::cout <<
std::format(...) calls (e.g., the line formatting "p[{}] = {}, p2[{}] = {},
p_true[{}] = {}" with variables k, p[k], p2[k], p_true[k]) are missing a
trailing newline; update each such std::cout << std::format(...) invocation to
include a newline (either append "\n" inside the format string or add << '\n'
after the formatted string) so each diagnostic ends with a newline—apply the
same fix to the other identical occurrences in the file.
In `@test/Core/Equation/PeriodicEqnTest.cpp`:
- Around line 97-101: The diagnostic std::cerr outputs in PeriodicEqnTest (the
std::format calls that print "Check fail: res = nan @ {}", and "Check fail: res
= {} / {} @ {}" using c_res, rel_res, i) are missing trailing newlines; update
those statements to terminate the output (e.g., append "\n" inside the format
string or stream a '\n' / std::endl after the formatted string) so each error
message prints on its own line and avoids run-together output.
In `@test/Core/Field/CartesianFieldMPITest.cpp`:
- Around line 13-21: Add the missing <cstring> include so std::memcpy is
available for the Int::deserialize method; update the top-of-file includes
(where other headers like <format> and <tuple> are listed) to add `#include`
<cstring> so module builds and stricter compilers can find std::memcpy used in
Int::deserialize.
In `@test/Utils/HDF5StreamTest.cpp`:
- Around line 13-19: Add an explicit `#include` <filesystem> to
test/Utils/HDF5StreamTest.cpp because std::filesystem::exists() is used later
(around lines 62–63) and the opflow module (import opflow or `#include` <OpFlow>)
does not re-export <filesystem>; insert the include alongside the other headers
at the top of the file so std::filesystem symbols are available for the
HDF5Stream tests.
🧹 Nitpick comments (17)
src/Core/Operator/FDMOperators/D1WENO53Upwind.hpp (1)
99-99: Consider removing the unuseddimvariable entirely.The
[[maybe_unused]]attribute correctly suppresses the warning, but sincedimis never used in this function, removing it would be cleaner than silencing the warning. Ifdimis intended for future use, the current approach is acceptable.♻️ Optional: Remove unused variable
template <CartesianFieldExprType E> static void prepare(const Expression<D1WENO53Upwind, E>& expr) { - [[maybe_unused]] constexpr auto dim = internal::CartesianFieldExprTrait<E>::dim; expr.initPropsFrom(expr.arg1);src/Core/BC/NeumBC.hpp (1)
21-23: Minor style inconsistency with other BC files.The module export macro is placed on a separate line here, while other files (e.g.,
BCBase.hpp,DircBC.hpp,ProxyBC.hpp) useOPFLOW_MODULE_EXPORT namespace OpFlow {on a single line. If this separation is intentional for module compilation fixes, consider applying consistently across all BC headers for uniformity.src/Utils/Writers/TecplotSZPLTStream.hpp (2)
91-106: Theis_doublevariable is declared but unused.The variable
is_double(line 94) is computed from the template parameterisDoublebut is never used in this function. It appears to be vestigial code, possibly from an earlier implementation that passed it to a TecIO function.The
(void) stat;pattern (line 100) is correctly used to silence unused variable warnings in Release builds where assertions are typically disabled.🧹 Optional cleanup: remove unused variable
int file_format = 1, // 0: Tecplot binary (.plt), 1: Tecplot subzone (.szplt) file_type = 0,// 0: full, 1: grid, 2: solution debug = 0; // 0: no-debug, 1: debug - [[maybe_unused]] int is_double = isDouble ? 1 : 0;// 0: f32, 1: f64
220-229: Duplicate initialization logic could be extracted.This code block (lines 220-229) is nearly identical to lines 91-100 in
operator<<. The same unusedis_doublevariable issue applies here. Consider extracting the common file initialization logic into a private helper method to reduce duplication and ensure consistent behavior.benchmark/Core/LaplaceOp.cpp (1)
49-91: Consider gating the 3D benchmark with a build flag instead of a block comment.
This keeps it easy to re-enable while defaulting to off.If there’s already a benchmark toggle macro in the codebase, please reuse it.
Optional refactor
-/* +#if defined(OPFLOW_ENABLE_3D_BENCH) static void Laplace_3d(benchmark::State& state) { ... } static void Laplace_3d_Params(benchmark::internal::Benchmark* b) { ... } -//BENCHMARK(Laplace_3d)->Apply(Laplace_3d_Params)->UseRealTime(); -*/ +BENCHMARK(Laplace_3d)->Apply(Laplace_3d_Params)->UseRealTime(); +#endifbenchmark/Core/UniFieldAssign.cpp (1)
72-104: Consider adding a comment explaining why the 3D benchmark is disabled.The 3D benchmark code is commented out without explanation. Adding a brief comment (e.g.,
// Disabled due to module compilation issues - see issue#XX``) would help future maintainers understand the intent and track when it can be re-enabled.src/Core/Field/MeshBased/SemiStructured/CartAMRField.hpp (1)
246-247: Reuse the computed range instead of recomputing it.You can drop the unused variable and avoid the second
DS::commonRangecall.♻️ Suggested change
- [[maybe_unused]] auto _r = DS::commonRange(bc_r, this->localRanges[l][r_n]); - rangeFor_s(DS::commonRange(bc_r, this->localRanges[l][r_n]), [&](auto&& i) { + auto r = DS::commonRange(bc_r, this->localRanges[l][r_n]); + rangeFor_s(r, [&](auto&& i) { // copy from other fine cells auto other_i = i; other_i.p = r_n; this->operator[](i) = this->operator[](other_i); });src/DataStructures/StencilPad.hpp (1)
160-171: Use the size parameter to validate deserialization bounds.
Ignoring the buffer length risks out-of-bounds reads on corrupted inputs.Proposed hardening
- void deserialize(const std::byte* data, std::size_t) override { - const std::byte* ptr = data; - int size_ = *(int*) ptr; + void deserialize(const std::byte* data, std::size_t size) override { + OP_ASSERT_MSG(size >= sizeof(int), "deserialize: buffer too small"); + const std::byte* ptr = data; + int size_ = *(int*) ptr; + OP_ASSERT_MSG(size_ >= 0, "deserialize: negative size"); + const auto expected = + sizeof(int) + static_cast<std::size_t>(size_) * (sizeof(Idx) + sizeof(Real)) + sizeof(Real); + OP_ASSERT_MSG(size >= expected, "deserialize: buffer too small"); ptr += sizeof(int);src/Utils/Writers/IOGroup.hpp (1)
26-26: Consider using[[maybe_unused]]for consistency.The comment-style parameter suppression works, but
[[maybe_unused]]would be more consistent with other changes in this PR (e.g.,VTKAMRStream.hpp,HDF5Stream.hpp).Suggested change
- virtual void read(const TimeStamp& /* t */) {} + virtual void read([[maybe_unused]] const TimeStamp& t) {}examples/CMakeLists.txt (1)
20-27: Good: Architecture-specific-mfmaflag handling, but indentation is inconsistent.The conditional
-mfmaflag addition correctly handles cross-platform builds (ARM/Apple Silicon doesn't support this x86 instruction). However, lines 21-24 lack the indentation used elsewhere in theelseifblock (e.g., lines 25-26 are indented with 4 spaces).🔧 Proposed fix for consistent indentation
elseif (CMAKE_CXX_COMPILER_ID MATCHES "Clang") -string(APPEND OPFLOW_EXAMPLE_CXX_FLAGS " -Wno-narrowing -Wno-deprecated-anon-enum-enum-conversion") -if (CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64|amd64|AMD64|i[3-6]86") - string(APPEND OPFLOW_EXAMPLE_CXX_FLAGS " -mfma") -endif () + string(APPEND OPFLOW_EXAMPLE_CXX_FLAGS " -Wno-narrowing -Wno-deprecated-anon-enum-enum-conversion") + if (CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64|amd64|AMD64|i[3-6]86") + string(APPEND OPFLOW_EXAMPLE_CXX_FLAGS " -mfma") + endif () string(APPEND OPFLOW_EXAMPLE_CXX_FLAGS_DEBUG "${OPFLOW_EXAMPLE_CXX_FLAGS} -O0 -g") string(APPEND OPFLOW_EXAMPLE_CXX_FLAGS_RELEASE "${OPFLOW_EXAMPLE_CXX_FLAGS} -O3 -fopenmp-simd -DNDEBUG") endif ()src/DataStructures/Index/MDIndex.hpp (1)
62-64: Consider usingstd::size_tfor consistency.Line 63 uses
auto i = 0which deduces toint, whiledisstd::size_t. This creates a signed/unsigned comparison. For consistency with the other loops in this file that were updated to usestd::size_t, consider updating this loop as well.Suggested fix
constexpr void set(const Meta::BracketIndexable auto& o) { - for (auto i = 0; i < d; ++i) this->idx[i] = o[i]; + for (std::size_t i = 0; i < d; ++i) this->idx[i] = o[i]; }benchmark/Core/AMRMeshBuild.cpp (1)
21-21: Consider removing commented-out code entirely.The
Fieldtype alias was unused in these mesh-building benchmarks. While commenting it out fixes the unused typedef warning, consider removing the lines entirely to keep the code clean. Commented-out code can be retrieved from version control if needed.Suggested fix
for (auto _ : state) { using Mesh = CartesianAMRMesh<Meta::int_<2>>; - //using Field = CartAMRField<Real, Mesh>; constexpr int n = 65, maxlevel = 4, ratio = 2, buffWidth = 5;for (auto _ : state) { using Mesh = CartesianAMRMesh<Meta::int_<3>>; - //using Field = CartAMRField<Real, Mesh>; constexpr int n = 17, maxlevel = 4, ratio = 4, buffWidth = 5;Also applies to: 72-72
src/CMakeLists.txt (1)
134-148: Avoid exporting-Werrorto consumers by default.
WhenOPFLOW_TARGET_SCOPEis INTERFACE,-Werrorpropagates to dependents and can break downstream builds. Consider gating Werror behind an option or keeping it PRIVATE to opflow’s own sources.♻️ One possible approach
+option(OPFLOW_WERROR "Treat warnings as errors for opflow" OFF) + if(CMAKE_CXX_COMPILER_ID MATCHES "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") - target_compile_options(opflow ${OPFLOW_TARGET_SCOPE} - -Wall - -Wextra - -Werror - -Wno-psabi - ) + target_compile_options(opflow ${OPFLOW_TARGET_SCOPE} + -Wall + -Wextra + -Wno-psabi + ) + if(OPFLOW_WERROR) + target_compile_options(opflow ${OPFLOW_TARGET_SCOPE} -Werror) + endif() endif()test/Core/Parallel/ParticleGuidedSplitStrategyTest.cpp (1)
74-74: Consider adding newline for cleaner test output.The debug output lacks a newline, which may cause output to run together. Consider appending
<< '\n'or usingstd::endlfor readability during test runs.♻️ Suggested improvement
- for (auto& m : map) std::cout << std::format("{}", m.toString()); + for (auto& m : map) std::cout << std::format("{}\n", m.toString());This applies similarly to lines 114 and 174.
test/Core/Equation/DircEqnTest.cpp (1)
99-103: Consider adding newlines for cleaner diagnostic output.The error messages will run together without newlines, making diagnostics harder to read. The referenced snippets in
AMGCLMPITest.cppandEqnHolderTest.cpphave the same issue.♻️ Suggested improvement
- std::cerr << std::format("Check fail: res = nan @ {}", i); + std::cerr << std::format("Check fail: res = nan @ {}\n", i); ret = false; } if (rel_res > rel) { - std::cerr << std::format("Check fail: res = {} / {} @ {}", c_res, rel_res, i); + std::cerr << std::format("Check fail: res = {} / {} @ {}\n", c_res, rel_res, i);test/Utils/HDF5StreamMPITest.cpp (2)
88-88: Consider adding newline for cleaner error output.♻️ Suggested improvement
- if (u[i] != v[i]) std::cerr << std::format("NOT EQUAL AT {}", i); + if (u[i] != v[i]) std::cerr << std::format("NOT EQUAL AT {}\n", i);
146-146: Consider adding newline for cleaner debug output.♻️ Suggested improvement
- std::cout << std::format("local range = {}", u.localRange.toString()); + std::cout << std::format("local range = {}\n", u.localRange.toString());
| - name: Checkout | ||
| uses: actions/checkout@master | ||
| with: | ||
| submodules: recursive | ||
| fetch-depth: 0 | ||
| - name: Install dependence |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/Build.yml | sed -n '40,50p'Repository: OpFlow-dev/OpFlow
Length of output: 488
🌐 Web query:
actions/checkout latest stable version 2024 2025
💡 Result:
- Latest stable in 2024:
actions/checkout@v4.2.2(released Oct 23, 2024 — year inferred from the repo’s release ordering/pagination). [1][2] - Latest stable in 2025:
actions/checkout@v6.0.0(released Nov 20, 2025 — year inferred from the repo’s release ordering). [3][4] - What to use in workflows (recommended): pin to the major (
@v6,@v5,@v4) unless you need an exact patch. [5]
Sources:
[1] https://github.com/actions/checkout/releases/tag/v4.2.2
[2] https://github.com/actions/checkout/releases (page showing v4.2.2)
[3] https://github.com/actions/checkout/releases/tag/v6.0.0
[4] https://github.com/actions/checkout/releases (page showing v6.0.0)
[5] https://github.com/marketplace/actions/checkout
Pin actions/checkout to a stable version instead of @master.
Using @master is mutable and a supply-chain risk. Update to @v6 (current stable version).
🛠️ Suggested fix
- uses: actions/checkout@master
+ uses: actions/checkout@v6📝 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.
| - name: Checkout | |
| uses: actions/checkout@master | |
| with: | |
| submodules: recursive | |
| fetch-depth: 0 | |
| - name: Install dependence | |
| - name: Checkout | |
| uses: actions/checkout@v6 | |
| with: | |
| submodules: recursive | |
| fetch-depth: 0 | |
| - name: Install dependence |
🤖 Prompt for AI Agents
In @.github/workflows/Build.yml around lines 42 - 47, Replace the mutable
reference to the checkout action so it uses a stable pinned release: update the
GitHub Actions step that sets "uses: actions/checkout@master" to use the stable
tag "actions/checkout@v6" (leave existing step keys like "name: Checkout",
"with: submodules: recursive", and "fetch-depth: 0" unchanged).
| # The specific values used for experimental CMake features. | ||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL "4.2") | ||
| set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD "d0edc3af-4c50-42ea-a356-e2862fe7a444") | ||
| set(CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE "73194a1d-c0b5-41b9-9190-a4512925e192") | ||
| else() | ||
| set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD | ||
| # This specific value changes as experimental support evolves. See | ||
| # `Help/dev/experimental.rst` in the CMake source corresponding to | ||
| # your CMake build for the exact value to use. | ||
| "a9e1cf81-9932-4810-974b-6eccaf14e457") | ||
| set(CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE 4bd552e2-b7fb-429a-ab23-c83ef53f3f13) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CMake CMAKE_EXPERIMENTAL_CXX_IMPORT_STD GUID values official documentation
💡 Result:
CMAKE_EXPERIMENTAL_CXX_IMPORT_STD is an experimental “gate” variable. CMake requires you to set it to a specific UUID/GUID string (and it may change between releases), and the official place it is documented is:
Help/dev/experimental.rstfrom the exact CMake version you are using (e.g., browse the CMake repo at thevX.Y.Ztag, notmaster). This is explicitly recommended by Kitware developers. [1]
Documented GUID values (examples from official CMake materials)
-
CMake 4.1.0-rc4 (
Help/dev/experimental.rst):
d0edc3af-4c50-42ea-a356-e2862fe7a444[2] -
CMake 4.0.2 (per Kitware/CMake developer on CMake Discourse):
a9e1cf81-9932-4810-974b-6eccaf14e457[1]
Related official docs (context)
- The
CXX_MODULE_STDproperty doc notes it only works when theCMAKE_EXPERIMENTAL_CXX_IMPORT_STDgate is enabled. [3] - The CMake C++ modules manual also reiterates that
import std;support is behind this experimental gate. [4]
References:
[1] CMake Discourse (Kitware developer guidance + 4.0.2 UUID) (discourse.cmake.org)
[2] CMake 4.1.0-rc4 Help/dev/experimental.rst (UUID) (fossies.org)
[3] CXX_MODULE_STD property docs (cmake.org)
[4] cmake-cxxmodules(7) manual (import std gate note) (cmake.org)
Citations:
- 1: https://discourse.cmake.org/t/import-std-appears-to-be-broken-in-release-4-0-2/14062?utm_source=openai
- 2: https://fossies.org/linux/misc/cmake-4.1.0-rc4.tar.gz/cmake-4.1.0-rc4/Help/dev/experimental.rst?utm_source=openai
- 3: https://cmake.org/cmake/help/v4.0/prop_tgt/CXX_MODULE_STD.html?utm_source=openai
- 4: https://cmake.org/cmake/help/latest/manual/cmake-cxxmodules.7.html?utm_source=openai
🌐 Web query:
CMake CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE GUID values documentation
💡 Result:
CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE is an “experimental gate” variable; CMake expects it to be set to a specific UUID/GUID string to enable the feature.
- Documented GUID (for the CMake source/doc set shown):
73194a1d-c0b5-41b9-9190-a4512925e192(this UUID may change across CMake versions). [1] - Where this GUID is documented/defined:
- In the CMake source-tree documentation:
Help/dev/experimental.rstunder “Build database support”. [1] - In CMake’s source code registry of experimental features (shows the same UUID tied to
CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE). [2]
- In the CMake source-tree documentation:
- Related docs:
CMAKE_EXPORT_BUILD_DATABASEandEXPORT_BUILD_DATABASEare only meaningful when this experimental gate is enabled. [3]
Usage example
cmake -S . -B build -DCMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE=73194a1d-c0b5-41b9-9190-a4512925e192Sources: [1] [2] [3]
🌐 Web query:
CMake 4.2 experimental features documentation GUIDs
💡 Result:
CMake’s experimental features are gated by CMAKE_EXPERIMENTAL_* variables that must be set to specific UUID/GUID values (documented in Help/dev/experimental.rst).
For CMake 4.2.x, the documented experimental feature UUIDs are:
| Experimental gate variable | UUID/GUID to set |
|---|---|
CMAKE_EXPERIMENTAL_EXPORT_PACKAGE_DEPENDENCIES |
1942b4fa-b2c5-4546-9385-83f254070067 [1] |
CMAKE_EXPERIMENTAL_EXPORT_PACKAGE_INFO |
b80be207-778e-46ba-8080-b23bba22639e [1] |
CMAKE_EXPERIMENTAL_FIND_CPS_PACKAGES |
e82e467b-f997-4464-8ace-b00808fff261 [1] |
CMAKE_EXPERIMENTAL_CXX_IMPORT_STD |
d0edc3af-4c50-42ea-a356-e2862fe7a444 [1] |
CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE |
73194a1d-c0b5-41b9-9190-a4512925e192 [1] |
CMAKE_EXPERIMENTAL_INSTRUMENTATION |
ec7aa2dc-b87f-45a3-8022-fe01c5f59984 [2] |
Note: the UUIDs are intentionally subject to change between CMake versions. [1]
Sources: [1] [2]
Verify CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE GUID for pre-4.2 CMake versions.
The CMake 4.2+ GUIDs are correct, and the CMAKE_EXPERIMENTAL_CXX_IMPORT_STD value for earlier versions matches CMake 4.0.2 documentation. However, the CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE GUID for CMake versions prior to 4.2 (4bd552e2-b7fb-429a-ab23-c83ef53f3f13) could not be verified against official CMake documentation. This may cause configure to fail on older CMake versions. Consult Help/dev/experimental.rst from your targeted pre-4.2 CMake version to confirm the correct GUID.
🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 18 - 29, The
CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE value for pre-4.2 CMake may be
incorrect; open the CMake dev documentation (Help/dev/experimental.rst)
corresponding to the pre-4.2 target and verify the correct GUID, then update the
CMAKE_EXPERIMENTAL_EXPORT_BUILD_DATABASE variable (the quoted value
"4bd552e2-b7fb-429a-ab23-c83ef53f3f13") in the CMakeLists to the verified GUID
(or add a clarifying comment linking to the exact dev doc/version if multiple
values exist) so configure won't fail on older CMake versions.
| auto root = make_result_root(); | ||
| Utils::TecplotASCIIStream uf("u.tec"), vf("v.tec"), wf("w.tec"), pf("p.tec"); |
There was a problem hiding this comment.
Unused variable: root is declared but never used.
The variable root is assigned the result of make_result_root() but is never referenced. The TecplotASCIIStream objects on line 204 use hardcoded filenames without the root prefix.
Either remove the unused variable or use it to prefix the output paths.
🐛 Option 1: Remove unused variable
- auto root = make_result_root();
Utils::TecplotASCIIStream uf("u.tec"), vf("v.tec"), wf("w.tec"), pf("p.tec");🐛 Option 2: Use root for output paths
auto root = make_result_root();
- Utils::TecplotASCIIStream uf("u.tec"), vf("v.tec"), wf("w.tec"), pf("p.tec");
+ Utils::TecplotASCIIStream uf(root + "u.tec"), vf(root + "v.tec"), wf(root + "w.tec"), pf(root + "p.tec");Note: This would also require creating the directory first.
📝 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.
| auto root = make_result_root(); | |
| Utils::TecplotASCIIStream uf("u.tec"), vf("v.tec"), wf("w.tec"), pf("p.tec"); | |
| Utils::TecplotASCIIStream uf("u.tec"), vf("v.tec"), wf("w.tec"), pf("p.tec"); |
| auto root = make_result_root(); | |
| Utils::TecplotASCIIStream uf("u.tec"), vf("v.tec"), wf("w.tec"), pf("p.tec"); | |
| auto root = make_result_root(); | |
| Utils::TecplotASCIIStream uf(root + "u.tec"), vf(root + "v.tec"), wf(root + "w.tec"), pf(root + "p.tec"); |
🤖 Prompt for AI Agents
In `@examples/LevelSet/UniLS.cpp` around lines 203 - 204, The variable root
(result of make_result_root()) is unused while the Tecplot files are written to
hardcoded names; either remove the unused root declaration or use it to prefix
the output filenames. If choosing to use root, update the
Utils::TecplotASCIIStream constructions (uf, vf, wf, pf) to build paths by
concatenating root with "u.tec", "v.tec", "w.tec", "p.tec" and ensure the
directory pointed to by root is created before constructing the streams (e.g.,
call the existing directory-creation helper or
std::filesystem::create_directories on root). If removing root, simply delete
the root = make_result_root() line.
| if(OPFLOW_ENABLE_MODULE) | ||
| set(OPFLOW_TARGET_SCOPE PUBLIC) | ||
| else() | ||
| set(OPFLOW_TARGET_SCOPE INTERFACE) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "CMakeLists.txt" src/Repository: OpFlow-dev/OpFlow
Length of output: 79
🏁 Script executed:
cat -n src/CMakeLists.txt | head -150Repository: OpFlow-dev/OpFlow
Length of output: 6815
Move the OPFLOW_TARGET_SCOPE definition before line 65 where it's first used.
target_include_directories() at line 65 and subsequent calls to target_compile_definitions() (lines 106, 110) and target_compile_options() (lines 135, 145) all expand ${OPFLOW_TARGET_SCOPE} before the variable is defined at lines 95-99. When CMake expands an undefined variable, it becomes an empty string, causing these target commands to receive no scope argument where PUBLIC or INTERFACE is expected. Move the scope definition block (lines 95-99) to execute before line 65.
🛠️ Suggested fix (move scope definition earlier)
+if(OPFLOW_ENABLE_MODULE)
+ set(OPFLOW_TARGET_SCOPE PUBLIC)
+else()
+ set(OPFLOW_TARGET_SCOPE INTERFACE)
+endif()
+
target_include_directories(opflow ${OPFLOW_TARGET_SCOPE}
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/../include>"
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/../src>"
"$<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/>"
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/src>")
-# ----------------------------------------------------------------------------
-# Misc definitions according to tweak options
-# ----------------------------------------------------------------------------
-if(OPFLOW_ENABLE_MODULE)
- set(OPFLOW_TARGET_SCOPE PUBLIC)
-else()
- set(OPFLOW_TARGET_SCOPE INTERFACE)
-endif()
+# ----------------------------------------------------------------------------
+# Misc definitions according to tweak options
+# ----------------------------------------------------------------------------🤖 Prompt for AI Agents
In `@src/CMakeLists.txt` around lines 95 - 99, The OPFLOW_TARGET_SCOPE variable is
defined too late and is expanded empty when used; move the conditional block
that sets OPFLOW_TARGET_SCOPE (the if(OPFLOW_ENABLE_MODULE) ... endif() block)
so it appears before the first use of ${OPFLOW_TARGET_SCOPE} in the
target_include_directories call, ensuring subsequent uses in
target_compile_definitions and target_compile_options see a proper PUBLIC or
INTERFACE scope.
| #if defined(OPFLOW_WITH_MPI) || __has_include(<mpi.h>) | ||
| #ifndef OMPI_SKIP_MPICXX | ||
| #define OMPI_SKIP_MPICXX 1 | ||
| #endif | ||
| #ifndef MPICH_SKIP_MPICXX | ||
| #define MPICH_SKIP_MPICXX 1 | ||
| #endif | ||
| #include <mpi.h> | ||
| #define OPFLOW_HAS_MPI 1 | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the Environment.hpp file around lines 16-25 and 38-45
if [ -f "src/Core/Environment.hpp" ]; then
echo "=== Lines 1-50 of Environment.hpp ==="
cat -n src/Core/Environment.hpp | head -50
else
echo "File not found at src/Core/Environment.hpp"
fd "Environment.hpp" --type f
fiRepository: OpFlow-dev/OpFlow
Length of output: 1852
🏁 Script executed:
# Search for CMake files defining OPFLOW_WITH_MPI
echo "=== Searching for CMakeLists.txt and cmake files ==="
fd "CMakeLists.txt|.*\.cmake$" -t f | head -20
echo -e "\n=== Searching for OPFLOW_WITH_MPI definition ==="
rg "OPFLOW_WITH_MPI" -t cmake -B 2 -A 2 | head -50Repository: OpFlow-dev/OpFlow
Length of output: 2982
🏁 Script executed:
# Check how OPFLOW_WITH_MPI is passed as compile definition
echo "=== Checking compile_definitions in CMakeLists.txt ==="
rg "add_compile_definitions|target_compile_definitions" CMakeLists.txt -B 2 -A 2 | grep -A 5 -B 5 "OPFLOW_WITH_MPI"
echo -e "\n=== Full context around OPFLOW_WITH_MPI usage in CMakeLists ==="
rg "OPFLOW_OPTION|OPFLOW_SINGLE_PRECISION|OPFLOW_WITH_MPI|OPFLOW_WITH_OPENMP" CMakeLists.txt -B 2 -A 2 | head -80Repository: OpFlow-dev/OpFlow
Length of output: 1550
🏁 Script executed:
# Check if MPI functions are only declared in specific conditions elsewhere
echo "=== Check for any guards around MPI_Init/Finalize calls ==="
rg "MPI_Init|MPI_Finalize" --type-add 'cpp:include:hpp' -B 3 -A 1
echo -e "\n=== Check if OPFLOW_HAS_MPI is used elsewhere ==="
rg "OPFLOW_HAS_MPI" --type-add 'cpp:include:hpp'Repository: OpFlow-dev/OpFlow
Length of output: 308
🏁 Script executed:
# Fix the rg search
echo "=== Check for MPI_Init/Finalize calls ==="
rg "MPI_Init|MPI_Finalize" -B 3 -A 1
echo -e "\n=== Check if OPFLOW_HAS_MPI is used elsewhere ==="
rg "OPFLOW_HAS_MPI"Repository: OpFlow-dev/OpFlow
Length of output: 50373
🏁 Script executed:
# Check OPFLOW_HAS_MPI usage more specifically
echo "=== Search for OPFLOW_HAS_MPI in src directory ==="
rg "OPFLOW_HAS_MPI" src/ -n
echo -e "\n=== Check if InitEnvironment/FinalizeEnvironment are actually called in main code ==="
rg "InitEnvironment|FinalizeEnvironment" src/ -B 1 -A 1Repository: OpFlow-dev/OpFlow
Length of output: 1108
MPI initialization may occur without proper linking when mpi.h is available but MPI is not explicitly enabled.
The condition at line 16 uses OR logic: #if defined(OPFLOW_WITH_MPI) || __has_include(<mpi.h>). Since OPFLOW_WITH_MPI defaults to OFF in CMake, and __has_include(<mpi.h>) only checks header availability (not linkage), this can set OPFLOW_HAS_MPI on systems with MPI headers but without MPI explicitly configured. This causes InitEnvironment/FinalizeEnvironment to call MPI_Init/MPI_Finalize without the MPI libraries being linked, resulting in linker or runtime errors. The auto-initialization via EnvironmentGardian makes this silent failure likely.
Change the OR to AND to require both explicit opt-in and header availability:
🛠️ Suggested guard tightening
-#if defined(OPFLOW_WITH_MPI) || __has_include(<mpi.h>)
+#if defined(OPFLOW_WITH_MPI) && __has_include(<mpi.h>)Also apply this change at lines 38 and 44 to gate initialization only on explicit opt-in:
-#if defined(OPFLOW_WITH_MPI) || defined(OPFLOW_HAS_MPI)
+#if defined(OPFLOW_WITH_MPI)🤖 Prompt for AI Agents
In `@src/Core/Environment.hpp` around lines 16 - 25, The preprocessor guard
currently enables OPFLOW_HAS_MPI when either OPFLOW_WITH_MPI is defined or mpi.h
exists; change the OR to AND so OPFLOW_HAS_MPI is defined only when both
OPFLOW_WITH_MPI is set and mpi.h is present to avoid calling MPI without
linkage. Update the three occurrences that set OPFLOW_HAS_MPI (the block around
the initial include and the guards referenced in the review) so the condition
reads require both OPFLOW_WITH_MPI and __has_include(<mpi.h>), and ensure
runtime calls in InitEnvironment/FinalizeEnvironment and the auto-init via
EnvironmentGardian remain gated by OPFLOW_HAS_MPI.
| #include <amgcl/amg.hpp> | ||
| #include <amgcl/backend/builtin.hpp> | ||
| #include <amgcl/coarsening/smoothed_aggregation.hpp> | ||
| #include <amgcl/make_solver.hpp> | ||
| #include <amgcl/relaxation/spai0.hpp> | ||
| #include <amgcl/solver/bicgstab.hpp> | ||
| #include <gmock/gmock.h> | ||
| #ifdef OPFLOW_USE_MODULE | ||
| import opflow; | ||
| #else | ||
| #include <OpFlow> | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the full test file to see line 55 and usage of zero_copy
cat -n test/Core/Equation/AMGCLTest.cpp | head -80Repository: OpFlow-dev/OpFlow
Length of output: 3200
🏁 Script executed:
# Search for zero_copy usage in the codebase
rg -n 'zero_copy' --include='*.cpp' --include='*.hpp' --include='*.h'Repository: OpFlow-dev/OpFlow
Length of output: 502
🏁 Script executed:
# Check if adapter/zero_copy.hpp exists in the repository
fd -a 'zero_copy.hpp'Repository: OpFlow-dev/OpFlow
Length of output: 122
🏁 Script executed:
# Search in AMGCL headers directory if it exists
fd -a 'adapter' -t dRepository: OpFlow-dev/OpFlow
Length of output: 109
🏁 Script executed:
# Check what includes the zero_copy.hpp header
rg -n 'zero_copy.hpp' external/amgcl/Repository: OpFlow-dev/OpFlow
Length of output: 490
🏁 Script executed:
# Check if make_solver.hpp includes adapter headers
cat external/amgcl/amgcl/make_solver.hpp | head -100Repository: OpFlow-dev/OpFlow
Length of output: 4498
🏁 Script executed:
# Check the adapter/zero_copy.hpp header to understand what it provides
head -50 external/amgcl/amgcl/adapter/zero_copy.hppRepository: OpFlow-dev/OpFlow
Length of output: 2208
Add #include <amgcl/adapter/zero_copy.hpp> to resolve compilation.
Line 55 uses amgcl::adapter::zero_copy(), but the adapter header is not included. The current includes (make_solver.hpp, amg.hpp, etc.) do not transitively provide it. Other AMGCL test files and the library documentation explicitly require this header.
Required fix
`#include` <amgcl/amg.hpp>
+#include <amgcl/adapter/zero_copy.hpp>
`#include` <amgcl/backend/builtin.hpp>
`#include` <amgcl/coarsening/smoothed_aggregation.hpp>📝 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.
| #include <amgcl/amg.hpp> | |
| #include <amgcl/backend/builtin.hpp> | |
| #include <amgcl/coarsening/smoothed_aggregation.hpp> | |
| #include <amgcl/make_solver.hpp> | |
| #include <amgcl/relaxation/spai0.hpp> | |
| #include <amgcl/solver/bicgstab.hpp> | |
| #include <gmock/gmock.h> | |
| #ifdef OPFLOW_USE_MODULE | |
| import opflow; | |
| #else | |
| #include <OpFlow> | |
| #endif | |
| `#include` <amgcl/amg.hpp> | |
| `#include` <amgcl/adapter/zero_copy.hpp> | |
| `#include` <amgcl/backend/builtin.hpp> | |
| `#include` <amgcl/coarsening/smoothed_aggregation.hpp> | |
| `#include` <amgcl/make_solver.hpp> | |
| `#include` <amgcl/relaxation/spai0.hpp> | |
| `#include` <amgcl/solver/bicgstab.hpp> | |
| `#include` <gmock/gmock.h> | |
| `#ifdef` OPFLOW_USE_MODULE | |
| import opflow; | |
| `#else` | |
| `#include` <OpFlow> | |
| `#endif` |
🤖 Prompt for AI Agents
In `@test/Core/Equation/AMGCLTest.cpp` around lines 15 - 26, Add the missing
include for the AMGCL zero-copy adapter: include the header that declares
amgcl::adapter::zero_copy by adding `#include` <amgcl/adapter/zero_copy.hpp> to
the top of AMGCLTest.cpp so calls to amgcl::adapter::zero_copy() (used later
around line 55) compile; ensure it is placed with the other amgcl includes (near
amgcl/make_solver.hpp or amgcl/amg.hpp).
| std::cout << std::format("p[{}] = {}, p2[{}] = {}, p_true[{}] = {}", k, p[k], k, p2[k], k, | ||
| p_true[k]); |
There was a problem hiding this comment.
Missing newline in diagnostic output.
The std::format output lacks a trailing newline, which can cause output to run together. The same issue appears at lines 172-173 and 264-265.
Proposed fix
- std::cout << std::format("p[{}] = {}, p2[{}] = {}, p_true[{}] = {}", k, p[k], k, p2[k], k,
- p_true[k]);
+ std::cout << std::format("p[{}] = {}, p2[{}] = {}, p_true[{}] = {}\n", k, p[k], k, p2[k], k,
+ p_true[k]);📝 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.
| std::cout << std::format("p[{}] = {}, p2[{}] = {}, p_true[{}] = {}", k, p[k], k, p2[k], k, | |
| p_true[k]); | |
| std::cout << std::format("p[{}] = {}, p2[{}] = {}, p_true[{}] = {}\n", k, p[k], k, p2[k], k, | |
| p_true[k]); |
🤖 Prompt for AI Agents
In `@test/Core/Equation/EqnSetMPITest.cpp` around lines 115 - 116, The diagnostic
prints using the std::cout << std::format(...) calls (e.g., the line formatting
"p[{}] = {}, p2[{}] = {}, p_true[{}] = {}" with variables k, p[k], p2[k],
p_true[k]) are missing a trailing newline; update each such std::cout <<
std::format(...) invocation to include a newline (either append "\n" inside the
format string or add << '\n' after the formatted string) so each diagnostic ends
with a newline—apply the same fix to the other identical occurrences in the
file.
| std::cerr << std::format("Check fail: res = nan @ {}", i); | ||
| ret = false; | ||
| } | ||
| if (rel_res > rel) { | ||
| std::print(std::cerr, "Check fail: res = {} / {} @ {}", c_res, rel_res, i); | ||
| std::cerr << std::format("Check fail: res = {} / {} @ {}", c_res, rel_res, i); |
There was a problem hiding this comment.
Missing newline in diagnostic output.
Similar to other test files, the formatted error messages lack trailing newlines, which can cause output to run together.
Proposed fix
if (std::isnan(c_res)) {
- std::cerr << std::format("Check fail: res = nan @ {}", i);
+ std::cerr << std::format("Check fail: res = nan @ {}\n", i);
ret = false;
}
if (rel_res > rel) {
- std::cerr << std::format("Check fail: res = {} / {} @ {}", c_res, rel_res, i);
+ std::cerr << std::format("Check fail: res = {} / {} @ {}\n", c_res, rel_res, i);
ret = false;
}📝 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.
| std::cerr << std::format("Check fail: res = nan @ {}", i); | |
| ret = false; | |
| } | |
| if (rel_res > rel) { | |
| std::print(std::cerr, "Check fail: res = {} / {} @ {}", c_res, rel_res, i); | |
| std::cerr << std::format("Check fail: res = {} / {} @ {}", c_res, rel_res, i); | |
| if (std::isnan(c_res)) { | |
| std::cerr << std::format("Check fail: res = nan @ {}\n", i); | |
| ret = false; | |
| } | |
| if (rel_res > rel) { | |
| std::cerr << std::format("Check fail: res = {} / {} @ {}\n", c_res, rel_res, i); | |
| ret = false; | |
| } |
🤖 Prompt for AI Agents
In `@test/Core/Equation/PeriodicEqnTest.cpp` around lines 97 - 101, The diagnostic
std::cerr outputs in PeriodicEqnTest (the std::format calls that print "Check
fail: res = nan @ {}", and "Check fail: res = {} / {} @ {}" using c_res,
rel_res, i) are missing trailing newlines; update those statements to terminate
the output (e.g., append "\n" inside the format string or stream a '\n' /
std::endl after the formatted string) so each error message prints on its own
line and avoids run-together output.
| #include <format> | ||
| #include <gmock/gmock.h> | ||
| #include <print> | ||
| #include <iostream> | ||
| #include <tuple> | ||
| #ifdef OPFLOW_USE_MODULE | ||
| import opflow; | ||
| #else | ||
| #include <OpFlow> | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat test/Core/Field/CartesianFieldMPITest.cppRepository: OpFlow-dev/OpFlow
Length of output: 8457
Add <cstring> header for std::memcpy usage in the deserializer.
The std::memcpy call in the Int::deserialize method (line 208) requires an explicit <cstring> include. Module builds are stricter about transitive includes and will fail to compile without it.
Proposed fix
+#include <cstring>
`#include` <format>📝 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.
| #include <format> | |
| #include <gmock/gmock.h> | |
| #include <print> | |
| #include <iostream> | |
| #include <tuple> | |
| #ifdef OPFLOW_USE_MODULE | |
| import opflow; | |
| #else | |
| #include <OpFlow> | |
| #endif | |
| `#include` <cstring> | |
| `#include` <format> | |
| `#include` <gmock/gmock.h> | |
| `#include` <iostream> | |
| `#include` <tuple> | |
| `#ifdef` OPFLOW_USE_MODULE | |
| import opflow; | |
| `#else` | |
| `#include` <OpFlow> | |
| `#endif` |
🤖 Prompt for AI Agents
In `@test/Core/Field/CartesianFieldMPITest.cpp` around lines 13 - 21, Add the
missing <cstring> include so std::memcpy is available for the Int::deserialize
method; update the top-of-file includes (where other headers like <format> and
<tuple> are listed) to add `#include` <cstring> so module builds and stricter
compilers can find std::memcpy used in Int::deserialize.
| #include <format> | ||
| #include <gmock/gmock.h> | ||
| #ifdef OPFLOW_USE_MODULE | ||
| import opflow; | ||
| #else | ||
| #include <OpFlow> | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect the test file
fd HDF5StreamTest.cppRepository: OpFlow-dev/OpFlow
Length of output: 90
🏁 Script executed:
# Check the file size and read the relevant sections
wc -l test/Utils/HDF5StreamTest.cppRepository: OpFlow-dev/OpFlow
Length of output: 94
🏁 Script executed:
# Read the full test file
cat -n test/Utils/HDF5StreamTest.cppRepository: OpFlow-dev/OpFlow
Length of output: 4307
🏁 Script executed:
# Search for opflow module definition files
fd -e cppm -e ixx -e mpp | head -20Repository: OpFlow-dev/OpFlow
Length of output: 91
🏁 Script executed:
# Check the size and contents of the opflow module
wc -l src/opflow.cppmRepository: OpFlow-dev/OpFlow
Length of output: 79
🏁 Script executed:
# Read the opflow module definition
cat -n src/opflow.cppmRepository: OpFlow-dev/OpFlow
Length of output: 705
🏁 Script executed:
# Check if there are other opflow module partitions
fd -e cppm -e ixx | xargs grep -l "module opflow"Repository: OpFlow-dev/OpFlow
Length of output: 76
🏁 Script executed:
# Check what's in the OpFlow header that's included in the module
fd -name "OpFlow" -type f | head -5Repository: OpFlow-dev/OpFlow
Length of output: 291
🏁 Script executed:
# Check what's in the OpFlow header that's included in the module
fd "OpFlow" -type f | head -5Repository: OpFlow-dev/OpFlow
Length of output: 231
🏁 Script executed:
# Check what's in the OpFlow header that's included in the module
find . -name "OpFlow" -type f | head -5Repository: OpFlow-dev/OpFlow
Length of output: 77
🏁 Script executed:
# Check if OpFlow is a directory or file
ls -la ./include/OpFlowRepository: OpFlow-dev/OpFlow
Length of output: 125
🏁 Script executed:
# Read the OpFlow main header
cat -n ./include/OpFlowRepository: OpFlow-dev/OpFlow
Length of output: 676
🏁 Script executed:
# Check what's exported from the Utils header
find ./include -name "Utils" -type dRepository: OpFlow-dev/OpFlow
Length of output: 43
🏁 Script executed:
# Check for Utils header file
find ./include -name "Utils*" -type f | head -10Repository: OpFlow-dev/OpFlow
Length of output: 76
🏁 Script executed:
# Check if Utils is a directory
ls -la ./include/UtilsRepository: OpFlow-dev/OpFlow
Length of output: 125
🏁 Script executed:
# Read the Utils header
cat -n ./include/UtilsRepository: OpFlow-dev/OpFlow
Length of output: 1736
Add explicit #include <filesystem> for module builds.
Line 62–63 uses std::filesystem::exists(). While the opflow module includes <filesystem> in its preamble, it is not re-exported; with OPFLOW_USE_MODULE, the module provides no transitive access to std::filesystem symbols, causing a compile error. Add an explicit include.
Suggested fix
`#include` <format>
+#include <filesystem>
`#include` <gmock/gmock.h>📝 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.
| #include <format> | |
| #include <gmock/gmock.h> | |
| #ifdef OPFLOW_USE_MODULE | |
| import opflow; | |
| #else | |
| #include <OpFlow> | |
| #endif | |
| `#include` <format> | |
| `#include` <filesystem> | |
| `#include` <gmock/gmock.h> | |
| `#ifdef` OPFLOW_USE_MODULE | |
| import opflow; | |
| `#else` | |
| `#include` <OpFlow> | |
| `#endif` |
🤖 Prompt for AI Agents
In `@test/Utils/HDF5StreamTest.cpp` around lines 13 - 19, Add an explicit `#include`
<filesystem> to test/Utils/HDF5StreamTest.cpp because std::filesystem::exists()
is used later (around lines 62–63) and the opflow module (import opflow or
`#include` <OpFlow>) does not re-export <filesystem>; insert the include alongside
the other headers at the top of the file so std::filesystem symbols are
available for the HDF5Stream tests.
1 participant
Summary by CodeRabbit
New Features
Documentation
Build & Infrastructure