direct imports / highfive full inc path #1111
Conversation
📝 WalkthroughWalkthroughThis pull request reorganizes cppdict include paths across the codebase and updates CMake build configuration. It replaces external include directives with local ones, adds cppdict linking to build targets, removes public include directory exposures, and configures properties for the Python module. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/restarts/restarts_props.hpp (1)
4-7: Restore<string>include to keep this header self-containedSwitching to
#include "dict.hpp"is consistent with the cppdict changes, butRestartsPropertiesstill usesstd::stringinFileAttributesand this header no longer includes<string>itself. That makes it dependent on transitive includes fromdict.hpp.Consider adding
<string>back explicitly:-#include <vector> - -#include "dict.hpp" +#include <vector> +#include <string> + +#include "dict.hpp"This keeps
restarts_props.hppself-contained regardless of howdict.hppevolves.Also applies to: 12-13
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
res/cmake/dep/highfive.cmake(1 hunks)src/amr/CMakeLists.txt(1 hunks)src/core/CMakeLists.txt(1 hunks)src/core/errors.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(1 hunks)src/diagnostic/diagnostic_props.hpp(1 hunks)src/diagnostic/diagnostics.hpp(1 hunks)src/initializer/CMakeLists.txt(1 hunks)src/initializer/data_provider.hpp(1 hunks)src/initializer/dictator.cpp(1 hunks)src/phare_core.hpp(1 hunks)src/python3/data_wrangler.hpp(1 hunks)src/restarts/restarts.hpp(1 hunks)src/restarts/restarts_props.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit configuration file
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/diagnostic/diagnostics.hppsrc/restarts/restarts_props.hppsrc/initializer/data_provider.hppsrc/python3/data_wrangler.hppsrc/core/errors.hppsrc/restarts/restarts.hppsrc/diagnostic/diagnostic_model_view.hppsrc/diagnostic/diagnostic_props.hppsrc/phare_core.hpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
📚 Learning: 2025-07-09T17:18:05.771Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/amr/CMakeLists.txt:103-105
Timestamp: 2025-07-09T17:18:05.771Z
Learning: In the PHARE project, `HighFive` is a CMake option defined in `res/cmake/options.cmake` as `option(HighFive "Build with highfive usage" ON)`, not a target that would be created by find_package(). The condition `if (HighFive)` correctly checks this option.
Applied to files:
src/diagnostic/diagnostics.hppres/cmake/dep/highfive.cmakesrc/core/CMakeLists.txt
📚 Learning: 2025-01-16T10:45:48.912Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 943
File: res/cmake/dep/highfive.cmake:7-7
Timestamp: 2025-01-16T10:45:48.912Z
Learning: The HighFive library development has moved from BlueBrain/HighFive to highfive-devs/highfive as of December 2024 due to the conclusion of the Blue Brain Project.
Applied to files:
res/cmake/dep/highfive.cmake
📚 Learning: 2025-02-07T14:35:14.630Z
Learnt from: nicolasaunai
Repo: PHAREHUB/PHARE PR: 591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, `lowResourceTests` and `highResourceTests` options must be mutually exclusive and cannot be enabled simultaneously.
Applied to files:
src/core/CMakeLists.txt
📚 Learning: 2025-11-10T09:37:57.021Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1084
File: src/diagnostic/diagnostic_model_view.hpp:219-221
Timestamp: 2025-11-10T09:37:57.021Z
Learning: In PHARE, temporary fields like tmpField_, tmpVec_, and tmpTensor_ in src/diagnostic/diagnostic_model_view.hpp use a name-based resource allocation pattern. Fields with names "PHARE_sumField", "PHARE_sumVec", and "PHARE_sumTensor" are allocated globally elsewhere in the resources manager (e.g., in src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp), and components reference the same storage through identically-named field objects. These do not require local allocation via rm.enumerate().
Applied to files:
src/diagnostic/diagnostic_model_view.hpp
🔇 Additional comments (12)
res/cmake/dep/highfive.cmake (1)
18-19: HighFive include path wiring looks fineAdding
include_directories(${HIGHFIVE_SRC}/include)beforeadd_subdirectory(${HIGHFIVE_SRC})is consistent with using HighFive as an embedded subproject and should make its public headers reliably visible to dependents in this directory scope.src/python3/data_wrangler.hpp (1)
17-17: dict.hpp include aligns with cppdict usageSwitching to
#include "dict.hpp"here matches the rest of the codebase’s use ofPHAREDictHandler/cppdictand keeps the dependency localized without touching the class logic.src/diagnostic/diagnostic_props.hpp (1)
9-9: Header now correctly pulls in cppdict definitionsIncluding
dict.hpphere makes thecppdict::Dicttypedefs self-contained and consistent with the include-path changes elsewhere in the PR.src/core/errors.hpp (1)
12-24: cppdict dependency is now explicitly includedBringing in
dict.hppmakes theDictionaryException::Dict_t = cppdict::Dict<std::string>alias properly self-contained in this header, with no changes to existing behavior.src/initializer/dictator.cpp (1)
2-3: Local dict include matches cppdict usage in bindingsIncluding
dict.hpphere correctly coverscppdict::addand the PHARE dict handler used by the pybind11 bindings, without altering module behavior.src/amr/CMakeLists.txt (1)
88-103: Linkingcppdictintophare_amrlooks appropriateAdding
cppdictas a PUBLIC dependency of the${PROJECT_NAME}amr library matches the move todict.hpp-based types in headers that downstream targets may include, and it fits naturally with the existing link set.src/restarts/restarts.hpp (1)
10-10: LGTM!The include path simplification from
"cppdict/include/dict.hpp"to"dict.hpp"is clean and aligns with the explicit cppdict linkage added in the CMakeLists.txt files.src/phare_core.hpp (1)
18-18: LGTM!The include path update is consistent with the PR's refactoring objectives.
src/diagnostic/diagnostic_model_view.hpp (1)
12-12: LGTM!The include path update is consistent with the PR's refactoring objectives.
src/diagnostic/diagnostics.hpp (1)
21-21: LGTM!The include path update is consistent with the PR's refactoring objectives.
src/initializer/CMakeLists.txt (2)
17-17: LGTM!Adding cppdict as a PUBLIC dependency is appropriate for phare_initializer, ensuring downstream targets have access to cppdict's include directories.
25-31: LGTM!The dictator Python module configuration is comprehensive and follows CMake best practices:
- PUBLIC linking to phare_initializer and pybind11::embed ensures proper dependency resolution
- Interprocedural optimization is enabled consistently with the parent library
- Library output directory is explicitly configured for Python module organization
- Link flags are properly preserved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/diagnostic/diagnostic_writer.hpp (1)
6-6: Same include path change as other files.This file makes the same include path change from
"cppdict/include/dict.hpp"to"dict.hpp". The verification script provided forsrc/core/utilities/types.hppwill confirm whether dict.hpp is properly resolvable across all affected files.src/restarts/restarts_props.hpp (1)
6-6: Same include path change pattern.This is the same include path update from
"cppdict/include/dict.hpp"to"dict.hpp"seen in other files. Refer to the verification script in the review ofsrc/core/utilities/types.hpp.src/core/CMakeLists.txt (1)
66-68: Change cppdict linkage from PRIVATE to PUBLIC.The cppdict library is linked as PRIVATE, but phare_core's public headers (e.g.,
src/phare_core.hppandsrc/core/errors.hpp) exposecppdict::Dicttypes and include"dict.hpp". When a library's public API exposes types from a dependency, that dependency must be linked as PUBLIC so downstream targets can access the necessary include directories.Apply this diff:
-target_link_libraries(${PROJECT_NAME} PRIVATE phare_initializer MPI::MPI_C cppdict +target_link_libraries(${PROJECT_NAME} PRIVATE phare_initializer MPI::MPI_C + PUBLIC cppdict PUBLIC ${PHARE_BASE_LIBS} )
🧹 Nitpick comments (1)
res/cmake/dep/highfive.cmake (1)
18-18: Let me check which targets actually depend on HighFive by examining the target hierarchy:Now let me verify whether all targets using HighFive are actually linked to the HighFive target:Perfect! The verification is complete. The web search confirms that HighFive creates interface targets with
target_include_directories(...INTERFACE...), and the recommended CMake pattern for using HighFive viaadd_subdirectoryistarget_link_libraries(foo HighFive).The review comment is valid. PHARE uses v3.0.0-beta2 (from the codebase), and the global
include_directories()call is indeed unnecessary since HighFive's CMakeLists exports its own target with interface includes. While PHARE's diagnostic module correctly links the HighFive target, the global include statement beforeadd_subdirectory()violates modern CMake practice by making includes available to all targets regardless of whether they actually depend on HighFive.Remove the global include_directories() call for HighFive and rely on target linkage instead.
The global
include_directories(${HIGHFIVE_SRC}/include)on line 18 is unnecessary. HighFive exports interface targets withtarget_include_directories(...INTERFACE...), and the documented CMake pattern usestarget_link_libraries(foo HighFive)to propagate includes automatically. PHARE already follows this correctly insrc/diagnostic/CMakeLists.txt:27where it links the HighFive target. Remove this line and ensure all targets using HighFive are explicitly linked to the HighFive target viatarget_link_libraries()rather than relying on the global include scope.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
res/cmake/dep/highfive.cmake(1 hunks)src/amr/CMakeLists.txt(1 hunks)src/core/CMakeLists.txt(1 hunks)src/core/errors.hpp(1 hunks)src/core/utilities/types.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(1 hunks)src/diagnostic/diagnostic_props.hpp(1 hunks)src/diagnostic/diagnostic_writer.hpp(1 hunks)src/diagnostic/diagnostics.hpp(1 hunks)src/initializer/CMakeLists.txt(1 hunks)src/initializer/data_provider.hpp(1 hunks)src/initializer/dictator.cpp(1 hunks)src/phare_core.hpp(1 hunks)src/python3/data_wrangler.hpp(1 hunks)src/restarts/restarts_props.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/diagnostic/diagnostics.hpp
- src/diagnostic/diagnostic_props.hpp
- src/initializer/dictator.cpp
- src/initializer/data_provider.hpp
- src/amr/CMakeLists.txt
- src/core/errors.hpp
- src/phare_core.hpp
- src/diagnostic/diagnostic_model_view.hpp
- src/python3/data_wrangler.hpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit configuration file
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/core/utilities/types.hppsrc/diagnostic/diagnostic_writer.hppsrc/restarts/restarts_props.hpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
📚 Learning: 2025-07-09T17:18:05.771Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/amr/CMakeLists.txt:103-105
Timestamp: 2025-07-09T17:18:05.771Z
Learning: In the PHARE project, `HighFive` is a CMake option defined in `res/cmake/options.cmake` as `option(HighFive "Build with highfive usage" ON)`, not a target that would be created by find_package(). The condition `if (HighFive)` correctly checks this option.
Applied to files:
src/core/CMakeLists.txtres/cmake/dep/highfive.cmake
📚 Learning: 2025-02-07T14:35:14.630Z
Learnt from: nicolasaunai
Repo: PHAREHUB/PHARE PR: 591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, `lowResourceTests` and `highResourceTests` options must be mutually exclusive and cannot be enabled simultaneously.
Applied to files:
src/core/CMakeLists.txt
📚 Learning: 2025-01-16T10:45:48.912Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 943
File: res/cmake/dep/highfive.cmake:7-7
Timestamp: 2025-01-16T10:45:48.912Z
Learning: The HighFive library development has moved from BlueBrain/HighFive to highfive-devs/highfive as of December 2024 due to the conclusion of the Blue Brain Project.
Applied to files:
res/cmake/dep/highfive.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (3)
src/initializer/CMakeLists.txt (2)
17-17: LGTM: PUBLIC linkage is appropriate for cppdict.The PUBLIC linkage of cppdict ensures that downstream targets have access to cppdict's include directories and usage requirements. This is the correct approach when a library's public API exposes types from a dependency.
25-31: LGTM: Python module configuration is appropriate.The configuration for the dictator Python module follows standard CMake practices:
- PUBLIC linkage ensures dependencies propagate correctly
- INTERPROCEDURAL_OPTIMIZATION respects project-wide settings
- Dedicated output directory organizes build artifacts
- Link flags follow project conventions
src/core/utilities/types.hpp (1)
20-20: Verify that cppdict v1.0.0's CMakeLists properly exports include directories.The include path was changed from the explicit
"cppdict/include/dict.hpp"to the relative"dict.hpp". This change depends entirely on cppdict's CMakeLists.txt correctly configuringtarget_include_directories()with PUBLIC scope so that linked targets inherit the include path.Since cppdict is an external dependency downloaded at build time, its CMakeLists cannot be verified in this sandbox. Before merging, confirm that:
- cppdict v1.0.0's CMakeLists exposes include directories via
target_include_directories(cppdict PUBLIC ...)orinclude_directories().- The build succeeds with the new relative include path in a full build environment.
1 participant
attempt to fix LSP/clangd
cleanup dict includes
todo: fix tests