resource manager variants handling #1015
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughAdded a public static helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as TestCase
participant RU as ResourceUser
participant RM as ResourcesManager
participant Var as VariantHelpers
participant Patch as PatchStore
Note over Test,RU: Prepare ResourceUser with vector<std::variant<...>>
Test->>RM: registerResources(RU)
RM->>RM: handle_sub_resources(registerResources, RU)
RM->>Var: (for runtime entries) get_as_tuple_or_throw / get_as_ref_or_throw
Var-->>RM: references to concrete sub-resources
loop For each Patch
Test->>RM: allocate(RU, Patch)
RM->>RM: handle_sub_resources(allocate, RU, Patch)
RM->>Var: extract variant element(s) for allocation
Var-->>RM: allocated sub-resources
Test->>RM: getIDs_(RU, Patch)
RM->>RM: handle_sub_resources(getIDs_, RU, Patch)
end
Note over Test,RM: Tests modify resource on one patch and verify persistence across patches
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
👮 Files not reviewed due to content moderation or server errors (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.hpp⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-01T09:41:40.618ZApplied to files:
🧬 Code graph analysis (3)src/core/utilities/variants.hpp (3)
tests/amr/resources_manager/test_resources_manager.cpp (1)
src/amr/resources_manager/resources_manager.hpp (4)
⏰ 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). (7)
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 (2)
tests/amr/resources_manager/test_resources_manager.cpp (2)
257-283: Test structures look good but consider adding validation.The test structures properly implement the required interfaces for variant-based resource management. However:
- The
FieldResourceinitializesField1Dwithnullptrwhich might cause issues if the field's data pointer is accessed during tests.- The
ResourceUser::get()method assumes exactly 2FieldResourceitems but doesn't validate this assumption.Consider adding validation:
auto get() { + if (resources.size() != 2) { + throw std::runtime_error("Expected exactly 2 resources"); + } return get_as_tuple_or_throw<FieldResource, FieldResource>(resources); }
302-340: Well-structured integration test for variant resources.The test properly exercises the variant-based resource management through the full lifecycle. Consider adding a comment to explain the significance of the test values.
auto dataOnPatch = resourcesManager.setOnPatch(*patch, resourceUser); auto&& [r0, r1] = resourceUser.get(); +// Set a test value at a specific index to verify data persistence r1.rho.data()[4] = 5;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/amr/resources_manager/resources_manager.hpp(7 hunks)tests/amr/resources_manager/test_resources_manager.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/resources_manager/resources_manager.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (4)
src/amr/resources_manager/resources_manager.hpp (3)
22-22: Include addition looks good.The tuple header is required for the
std::applyusage in the newhandle_sub_resourcesfunction.
168-170: Excellent refactoring to eliminate code duplication.The replacement of inline recursive logic with calls to
handle_sub_resourcesinregisterResources,allocate,getIDs_, andsetResources_improves code maintainability and consistency. All lambda captures and parameter forwarding are correctly implemented.Also applies to: 194-196, 339-339, 397-400
412-412: Minor formatting improvement.tests/amr/resources_manager/test_resources_manager.cpp (1)
11-14: Required includes for variant-based testing.The new includes are appropriate for the variant-based resource management tests.
Also applies to: 21-22
nicolasaunai
approved these changes
Jun 6, 2025
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * unpacked elements | ||
| */ | ||
|
|
||
| void static handle_sub_resources(auto fn, auto& obj, auto&&... args) |
Check notice
Code scanning / CodeQL
Unused static variable Note
| * unpacked elements | ||
| */ | ||
|
|
||
| void static handle_sub_resources(auto fn, auto& obj, auto&&... args) |
Check notice
Code scanning / CodeQL
Unused local variable Note
| std::vector<Resources> resources{FieldResource{}, VecFieldResource{}}; | ||
|
|
||
| { | ||
| auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| std::vector<Resources> resources{FieldResource{}, VecFieldResource{}}; | ||
|
|
||
| { | ||
| auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| for (auto const& patch : *hierarchy->getPatchLevel(iLevel)) | ||
| { | ||
| auto dataOnPatch = resourcesManager.setOnPatch(*patch, resourceUser); | ||
| auto&& [r0, r1] = resourceUser.get(); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| for (auto const& patch : *hierarchy->getPatchLevel(iLevel)) | ||
| { | ||
| auto dataOnPatch = resourcesManager.setOnPatch(*patch, resourceUser); | ||
| auto&& [r0, r1] = resourceUser.get(); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/utilities/variants.hpp (3)
68-68: Improve error message with type information.The generic error message makes debugging difficult when the function throws.
- throw std::runtime_error("No element in variant for type"); + throw std::runtime_error("No element in variant for type: " + std::string(typeid(Type).name()));
94-95: Enhance error message to indicate which type was not found.The generic error message doesn't help identify which type extraction failed.
- for_N<tuple_size>([&](auto i) { - if (std::get<i>(pointer_tuple) == nullptr) - throw std::runtime_error("No element in variant for type"); - }); + for_N<tuple_size>([&](auto i) { + if (std::get<i>(pointer_tuple) == nullptr) + { + using Type = std::tuple_element_t<i, Tuple>; + throw std::runtime_error("No element in variant for type at index " + std::to_string(i) + + ": " + std::string(typeid(Type).name())); + } + });
72-73: Document the ordering requirement and algorithm complexity.The comment emphasizes order requirements but doesn't explain the consequences or the algorithm's complexity.
-// ARGS MUST BE IN THE SAME ORDER AS VARIANT LIST TYPES!!!!! +// ARGS MUST BE IN THE SAME ORDER AS VARIANT LIST TYPES!!!!! +// The function assumes types appear in the same order in both the template arguments +// and the variant collection. Violating this assumption will result in runtime errors. +// Complexity: O(n×m) where n = variants.size() and m = sizeof...(Args)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/amr/resources_manager/resources_manager.hpp(6 hunks)src/core/utilities/variants.hpp(1 hunks)tests/amr/resources_manager/test_resources_manager.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/amr/resources_manager/test_resources_manager.cpp
- src/amr/resources_manager/resources_manager.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/core/utilities/variants.hpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
src/core/utilities/variants.hpp (2)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: src/amr/data/particles/refine/particles_data_split.hpp:181-182
Timestamp: 2024-10-16T09:54:01.455Z
Learning: The codebase uses C++17 features such as class template argument deduction, allowing template arguments to be omitted in declarations like `std::array`.
⏰ 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 (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
PhilipDeegan
force-pushed
the
rm_variants
branch
from
da99c68 to
720ecf2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/core/utilities/variants.hpp (2)
13-17: Remove unsafeconst_castinptr_or_null_for_type.
83-98:get_as_tuple_or_throw: error message & performance refinementstests/amr/resources_manager/test_resources_manager.cpp (1)
310-325: Add assertions to verify the helper functions work correctly.Additionally, to address the static analysis warnings about unused variables, consider using the extracted references or adding explicit assertions:
{ auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources); + // Verify extraction succeeded - accessing members would fail if wrong type + static_cast<void>(rho.rho); + static_cast<void>(B.B); } { auto& rho = get_as_ref_or_throw<FieldResource>(resources); + EXPECT_EQ(&rho, &std::get<FieldResource>(resources[0])); } { auto& B = get_as_ref_or_throw<VecFieldResource>(resources); + EXPECT_EQ(&B, &std::get<VecFieldResource>(resources[1])); }
🧹 Nitpick comments (4)
src/core/utilities/variants.hpp (3)
20-26: Fix typo in struct name:varient→variant.The struct name contains a typo that should be corrected for consistency with standard C++ terminology.
-template<typename... Ts> -struct varient_visitor_overloads : Ts... +template<typename... Ts> +struct variant_visitor_overloads : Ts... { using Ts::operator()...; }; template<typename... Ts> -varient_visitor_overloads(Ts&&...) -> varient_visitor_overloads<std::decay_t<Ts>...>; +variant_visitor_overloads(Ts&&...) -> variant_visitor_overloads<std::decay_t<Ts>...>;
32-32: Update to use corrected struct name.- return varient_visitor_overloads{ptr_or_null_for_type<Args>()..., + return variant_visitor_overloads{ptr_or_null_for_type<Args>()...,
70-70: Enhance error message with type information.The error message should include the type name for better debugging.
- throw std::runtime_error("No element in variant for type"); + throw std::runtime_error("No element in variant for type: " + std::string(typeid(Type).name()));tests/amr/resources_manager/test_resources_manager.cpp (1)
372-372: Remove or use the unused variabler0.The variable
r0is extracted but never used, triggering static analysis warnings.- auto&& [r0, r1] = resourceUser.get(); + auto&& [_, r1] = resourceUser.get();Or if you need both resources:
auto&& [r0, r1] = resourceUser.get(); + static_cast<void>(r0); // Explicitly mark as unused r1.rho.data()[4] = 5;Also applies to: 382-382
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/amr/resources_manager/resources_manager.hpp(5 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/variants.hpp(1 hunks)tests/amr/resources_manager/test_resources_manager.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/data/tensorfield/tensorfield.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/amr/resources_manager/resources_manager.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/core/utilities/variants.hpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
tests/amr/resources_manager/test_resources_manager.cpp (3)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE simulations, velocity calculations must account for per-population densities that can be strictly zero in given cells. Simple zero-return solutions for division by zero are inadequate as they don't reflect the correct physics.
Learnt from: nicolasaunai
PR: PHAREHUB/PHARE#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.
src/core/utilities/variants.hpp (2)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: src/amr/data/particles/refine/particles_data_split.hpp:181-182
Timestamp: 2024-10-16T09:54:01.455Z
Learning: The codebase uses C++17 features such as class template argument deduction, allowing template arguments to be omitted in declarations like `std::array`.
⏰ 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). (7)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (1)
tests/amr/resources_manager/test_resources_manager.cpp (1)
328-349: Well-structured test with proper assertions.The test correctly validates the
get_from_variantshelper function with appropriate assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/core/utilities/variants.hpp (2)
13-17: Remove unsafeconst_castindecay_to_ptr.The unconditional use of
const_castcan lead to undefined behavior if used on const objects. This violates const-correctness and could cause subtle bugs or crashes.
103-117: Missingthrowstatement causes undefined behavior.The function constructs an error message but never throws it, leading to undefined behavior when the required name is not found.
🧹 Nitpick comments (4)
src/core/utilities/variants.hpp (4)
19-27: Fix typo in struct name.The struct name
varient_visitor_overloadscontains a typo - it should bevariant_visitor_overloads.-template<typename... Ts> -struct varient_visitor_overloads : Ts... -{ - using Ts::operator()...; -}; - -template<typename... Ts> -varient_visitor_overloads(Ts&&...) -> varient_visitor_overloads<std::decay_t<Ts>...>; +template<typename... Ts> +struct variant_visitor_overloads : Ts... +{ + using Ts::operator()...; +}; + +template<typename... Ts> +variant_visitor_overloads(Ts&&...) -> variant_visitor_overloads<std::decay_t<Ts>...>;
29-34: Update function to use corrected struct name.This function references the misspelled
varient_visitor_overloadsstruct. Update it to use the corrected namevariant_visitor_overloads.template<typename... Args> auto constexpr _visit_ptr_overloads(std::tuple<Args...>*) { - return varient_visitor_overloads{decay_to_ptr<Args>()..., + return variant_visitor_overloads{decay_to_ptr<Args>()..., [](auto&) mutable -> void* { return nullptr; }}; }
62-70: Improve error message for better debugging.The generic error message makes debugging difficult. Consider including more specific information about the failed type lookup.
- throw std::runtime_error("No element in variant for type"); + throw std::runtime_error("No element in variant for type: " + std::string(typeid(Type).name()));
74-101: Consider performance optimization for large variant collections.The nested loops in this function result in O(N × M) complexity. For large variant lists, consider optimizing the type lookup mechanism.
Additionally, improve the error message to include more context:
- throw std::runtime_error("No element in variant for type"); + throw std::runtime_error("No element in variant for type at tuple position " + std::to_string(i));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/utilities/variants.hpp(1 hunks)tests/amr/resources_manager/test_resources_manager.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/amr/resources_manager/test_resources_manager.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
src/core/utilities/variants.hpp (2)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: src/amr/data/particles/refine/particles_data_split.hpp:181-182
Timestamp: 2024-10-16T09:54:01.455Z
Learning: The codebase uses C++17 features such as class template argument deduction, allowing template arguments to be omitted in declarations like `std::array`.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
🧬 Code Graph Analysis (1)
src/core/utilities/variants.hpp (2)
src/core/data/tensorfield/tensorfield.hpp (6)
for_N(84-88)for_N(89-93)i(179-179)i(179-179)i(180-180)i(180-180)tests/amr/resources_manager/test_resources_manager.cpp (2)
forward_as_tuple(284-284)forward_as_tuple(291-291)
⏰ 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). (7)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (5)
src/core/utilities/variants.hpp (5)
1-10: Header structure looks good.The header guards follow proper naming conventions and all necessary includes are present for the variant utilities functionality.
37-51: Well-implemented template metafunction.The
uniquetemplate struct andunique_tuplealias provide a clean way to filter duplicate types from template parameter packs. The implementation follows modern C++ best practices.
54-58: Clean implementation of visitor creation.The
visit_ptr_overloadsfunction effectively combines the unique type filtering with visitor creation. The use ofunique_tupleensures type safety.
120-125: Good use of modern C++ features.The variadic overload with the
requiresclause is well-implemented and provides a clean interface for extracting multiple elements.
130-134: Proper file structure closing.The namespace and header guard are properly closed.
PhilipDeegan
force-pushed
the
rm_variants
branch
from
9a25324 to
6cbddf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/utilities/variants.hpp (1)
115-117: Throw when a name is missing inget_from_variants.Falling off the end here just constructs a
std::runtime_errorand returns without a value, which is UB. Please actually throw.- if (start == variants.size()) - std::runtime_error("Required name not found in variants: " + arg.name()); + if (start == variants.size()) + throw std::runtime_error("Required name not found in variants: " + arg.name());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/amr/resources_manager/resources_manager.hpp(5 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/variants.hpp(1 hunks)tests/amr/resources_manager/test_resources_manager.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/data/tensorfield/tensorfield.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/amr/resources_manager/resources_manager.hppsrc/core/utilities/variants.hpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
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.
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: 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: 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.
📚 Learning: 2025-09-01T09:41:40.618Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1056
File: tests/core/numerics/interpolator/test_main.cpp:314-318
Timestamp: 2025-09-01T09:41:40.618Z
Learning: The PHARE project uses C++20, which makes the typename keyword optional for dependent types in obvious contexts. The project prefers minimal code without unnecessary keywords, so typename suggestions for dependent types should be avoided when the code compiles successfully in C++20.
Applied to files:
src/amr/resources_manager/resources_manager.hppsrc/core/utilities/variants.hpp
📚 Learning: 2025-04-17T14:41:53.643Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
Applied to files:
src/core/utilities/variants.hpp
🧬 Code graph analysis (1)
tests/amr/resources_manager/test_resources_manager.cpp (1)
src/core/utilities/variants.hpp (8)
get_as_tuple_or_throw(75-101)get_as_tuple_or_throw(75-75)get_as_ref_or_throw(63-70)get_as_ref_or_throw(63-63)get_from_variants(104-117)get_from_variants(104-104)get_from_variants(121-125)get_from_variants(121-122)
⏰ 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). (7)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
PhilipDeegan
force-pushed
the
rm_variants
branch
from
6cbddf1 to
0f9b875
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
roch is using this on his close branch so we might aswell get it in separately
allow conditional runtime resource allocation via variants