resource manager variants handling #1015
Conversation
📝 WalkthroughWalkthroughA static helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase
participant ResourceUser
participant ResourcesManager
participant Patch
participant Hierarchy
TestCase->>ResourceUser: Create instance (with variant resources)
TestCase->>ResourcesManager: Register ResourceUser
TestCase->>Hierarchy: Create patches
loop For each patch
TestCase->>ResourcesManager: Allocate ResourceUser on Patch
TestCase->>ResourcesManager: Set ResourceUser on Patch
TestCase->>ResourcesManager: Get ResourceUser from Patch
ResourcesManager->>ResourceUser: Access resources (via variant helpers)
ResourceUser->>TestCase: Return resource reference
TestCase->>ResourceUser: Modify resource data
end
TestCase->>ResourcesManager: Retrieve and verify resource data
Possibly related PRs
One-liner: The main PR and retrieved PR are directly related, as they both introduce the Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
| void static handle_sub_resources(auto fn, auto& obj, auto&&... args) | ||
| { | ||
| using ResourcesView = decltype(obj); | ||
|
|
||
| if constexpr (has_runtime_subresourceview_list<ResourcesView>::value) | ||
| { | ||
| for (auto& runtimeResource : obj.getRunTimeResourcesViewList()) | ||
| { | ||
| using RuntimeResource = decltype(runtimeResource); | ||
| if constexpr (has_sub_resources_v<RuntimeResource>) | ||
| { | ||
| fn(runtimeResource, args...); | ||
| } | ||
| else | ||
| { | ||
| std::visit([&](auto&& val) { fn(val, args...); }, runtimeResource); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if constexpr (has_compiletime_subresourcesview_list<ResourcesView>::value) | ||
| { | ||
| // unpack the tuple subResources and apply for each element registerResources() | ||
| // (recursively) | ||
| std::apply([&](auto&... subResource) { (fn(subResource, args...), ...); }, | ||
| obj.getCompileTimeResourcesViewList()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for the new helper function.
The handle_sub_resources function is a key part of the refactoring but lacks documentation. Please add a doc comment explaining:
- Purpose of the function
- Parameters (fn, obj, args)
- How it handles runtime vs compile-time sub-resources
- The variant handling logic
Additionally, ensure that C++20 is enabled in the build configuration since this function uses auto parameters.
+ /** @brief Traverse and apply a function to all sub-resources of a ResourcesView
+ *
+ * This helper function centralizes the logic for recursively handling both runtime
+ * and compile-time sub-resources. For runtime resources that are variants, it uses
+ * std::visit to handle the contained type.
+ *
+ * @param fn Callable to apply to each sub-resource
+ * @param obj The ResourcesView object containing sub-resources
+ * @param args Additional arguments to forward to fn
+ */
void static handle_sub_resources(auto fn, auto& obj, auto&&... args)📝 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.
| void static handle_sub_resources(auto fn, auto& obj, auto&&... args) | |
| { | |
| using ResourcesView = decltype(obj); | |
| if constexpr (has_runtime_subresourceview_list<ResourcesView>::value) | |
| { | |
| for (auto& runtimeResource : obj.getRunTimeResourcesViewList()) | |
| { | |
| using RuntimeResource = decltype(runtimeResource); | |
| if constexpr (has_sub_resources_v<RuntimeResource>) | |
| { | |
| fn(runtimeResource, args...); | |
| } | |
| else | |
| { | |
| std::visit([&](auto&& val) { fn(val, args...); }, runtimeResource); | |
| } | |
| } | |
| } | |
| if constexpr (has_compiletime_subresourcesview_list<ResourcesView>::value) | |
| { | |
| // unpack the tuple subResources and apply for each element registerResources() | |
| // (recursively) | |
| std::apply([&](auto&... subResource) { (fn(subResource, args...), ...); }, | |
| obj.getCompileTimeResourcesViewList()); | |
| } | |
| } | |
| /** @brief Traverse and apply a function to all sub-resources of a ResourcesView | |
| * | |
| * This helper function centralizes the logic for recursively handling both runtime | |
| * and compile-time sub-resources. For runtime resources that are variants, it uses | |
| * std::visit to handle the contained type. | |
| * | |
| * @param fn Callable to apply to each sub-resource | |
| * @param obj The ResourcesView object containing sub-resources | |
| * @param args Additional arguments to forward to fn | |
| */ | |
| void static handle_sub_resources(auto fn, auto& obj, auto&&... args) | |
| { | |
| using ResourcesView = decltype(obj); | |
| if constexpr (has_runtime_subresourceview_list<ResourcesView>::value) | |
| { | |
| for (auto& runtimeResource : obj.getRunTimeResourcesViewList()) | |
| { | |
| using RuntimeResource = decltype(runtimeResource); | |
| if constexpr (has_sub_resources_v<RuntimeResource>) | |
| { | |
| fn(runtimeResource, args...); | |
| } | |
| else | |
| { | |
| std::visit([&](auto&& val) { fn(val, args...); }, runtimeResource); | |
| } | |
| } | |
| } | |
| if constexpr (has_compiletime_subresourcesview_list<ResourcesView>::value) | |
| { | |
| std::apply([&](auto&... subResource) { (fn(subResource, args...), ...); }, | |
| obj.getCompileTimeResourcesViewList()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/amr/resources_manager/resources_manager.hpp around lines 128 to 156, add
a detailed doc comment above the handle_sub_resources function explaining its
purpose to apply a given function to all sub-resources of an object, describe
the parameters fn (function to apply), obj (resource container), and args
(additional arguments), clarify how it separately handles runtime sub-resources
via iteration and variant visitation, and compile-time sub-resources via tuple
unpacking, and mention the variant visitation logic used for runtime resources.
Also, verify that C++20 is enabled in the build configuration to support the use
of auto parameters in this function.
| TEST(usingResources, test_variants_helpers) | ||
| { | ||
| using Resources = std::variant<FieldResource, VecFieldResource>; | ||
|
|
||
| std::vector<Resources> resources{FieldResource{}, VecFieldResource{}}; | ||
|
|
||
| { | ||
| auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources); | ||
| } | ||
| { | ||
| auto& rho = get_as_ref_or_throw<FieldResource>(resources); | ||
| } | ||
| { | ||
| auto& B = get_as_ref_or_throw<VecFieldResource>(resources); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify the helper functions work correctly.
The test creates the resources and extracts them but doesn't verify that the extraction was successful or that the correct objects were retrieved.
Add assertions to ensure the helpers work correctly:
{
auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources);
+ EXPECT_NO_THROW(rho.rho.name());
+ EXPECT_NO_THROW(B.B.name());
}
{
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]));
}📝 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.
| TEST(usingResources, test_variants_helpers) | |
| { | |
| using Resources = std::variant<FieldResource, VecFieldResource>; | |
| std::vector<Resources> resources{FieldResource{}, VecFieldResource{}}; | |
| { | |
| auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources); | |
| } | |
| { | |
| auto& rho = get_as_ref_or_throw<FieldResource>(resources); | |
| } | |
| { | |
| auto& B = get_as_ref_or_throw<VecFieldResource>(resources); | |
| } | |
| } | |
| TEST(usingResources, test_variants_helpers) | |
| { | |
| using Resources = std::variant<FieldResource, VecFieldResource>; | |
| std::vector<Resources> resources{FieldResource{}, VecFieldResource{}}; | |
| { | |
| auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources); | |
| EXPECT_NO_THROW(rho.rho.name()); | |
| EXPECT_NO_THROW(B.B.name()); | |
| } | |
| { | |
| 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])); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/amr/resources_manager/test_resources_manager.cpp around lines 285 to
300, the test calls helper functions to extract resources but lacks assertions
to verify correctness. Add assertions after each extraction to check that the
returned references or tuples correspond to the expected resource types and
values, ensuring the helpers work as intended.
There was a problem hiding this comment.
the first two EXPECT_NO_THROW aren't really correct, as it's the call to get_as_tuple_or_throw that would throw
but the two EXPECT_EQs are are a good idea
There was a problem hiding this comment.
@PhilipDeegan You're absolutely right! The get_as_tuple_or_throw call itself is what would throw on type mismatch, not the subsequent .name() calls on successfully extracted objects. Here's the corrected suggestion keeping only the meaningful assertions:
TEST(usingResources, test_variants_helpers)
{
using Resources = std::variant<FieldResource, VecFieldResource>;
std::vector<Resources> resources{FieldResource{}, VecFieldResource{}};
{
auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources);
// The extraction itself validates the variant types are correct
}
{
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]));
}
}The EXPECT_EQ assertions properly verify that the helper functions return references to the correct variant objects.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -123,6 +124,36 @@ | |||
| * we ask for them in a tuple, and recursively call registerResources() for all of the | |||
| * unpacked elements | |||
| */ | |||
|
|
|||
| void static handle_sub_resources(auto fn, auto& obj, auto&&... args) | |||
Check notice
Code scanning / CodeQL
Unused static variable Note
| @@ -123,6 +124,36 @@ | |||
| * we ask for them in a tuple, and recursively call registerResources() for all of the | |||
| * 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
| auto const&& [rho, B] = get_as_tuple_or_throw<FieldResource, VecFieldResource>(resources); | ||
| } | ||
| { | ||
| auto& rho = get_as_ref_or_throw<FieldResource>(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)
|
|
||
| auto ptr_or_null = visit_ptr_overloads<Args...>(); | ||
|
|
||
| auto pointer_tuple = for_N<tuple_size>([&](auto i) mutable { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that for_N utility is available.
The function uses for_N and for_N_R_mode which are not defined in this file.
🏁 Script executed:
#!/bin/bash
# Description: Check if for_N utility is defined in the codebase
# Search for for_N definition
ast-grep --pattern 'for_N'
# Also check with ripgrep for any definitions
rg -A 5 'template.*for_N'Length of output: 2004
get_as_tuple_or_throw: error message & performance refinements
- The
for_Nandfor_N_R_modeutilities are defined insrc/core/utilities/types.hpp, so the dependency is already satisfied—no changes needed there. - In
src/core/utilities/variants.hppat line 95, the thrown exception is too generic. Enrich it with the missing type or tuple index, for example:throw std::runtime_error( "Variant extraction failed: type '" + std::string(typeid(T).name()) + "' not found at position " + std::to_string(i) );
- The current nested loops in
get_as_tuple_or_throwrun in O(N × M). For large variant lists, consider a single-pass lookup (e.g., building a compile-time or runtime map from type to index) to reduce it to O(N).
🤖 Prompt for AI Agents
In src/core/utilities/variants.hpp around lines 81 to 95, improve the exception
thrown in get_as_tuple_or_throw by including the missing type name and tuple
index in the error message for better clarity. Additionally, refactor the nested
loops performing type lookups to use a single-pass approach, such as a
compile-time or runtime map from type to index, to reduce the complexity from
O(N × M) to O(N) and enhance performance.
src/core/utilities/variants.hpp
Outdated
| template<typename T> | ||
| auto address_of_type_as_void_ptr() | ||
| { | ||
| return [](T& arg) mutable -> void* { return const_cast<std::decay_t<T>*>(&arg); }; | ||
| } |
There was a problem hiding this comment.
Remove unsafe const_cast in address_of_type_as_void_ptr.
The unconditional use of const_cast can lead to undefined behavior if used on const objects. This violates const-correctness and could cause subtle bugs or crashes.
Consider providing const and non-const overloads:
-template<typename T>
-auto address_of_type_as_void_ptr()
-{
- return [](T& arg) mutable -> void* { return const_cast<std::decay_t<T>*>(&arg); };
-}
+template<typename T>
+auto address_of_type_as_void_ptr()
+{
+ return [](T& arg) mutable -> void* { return &arg; };
+}
+
+template<typename T>
+auto address_of_type_as_const_void_ptr()
+{
+ return [](T const& arg) mutable -> void const* { return &arg; };
+}📝 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.
| template<typename T> | |
| auto address_of_type_as_void_ptr() | |
| { | |
| return [](T& arg) mutable -> void* { return const_cast<std::decay_t<T>*>(&arg); }; | |
| } | |
| template<typename T> | |
| auto address_of_type_as_void_ptr() | |
| { | |
| return [](T& arg) mutable -> void* { return &arg; }; | |
| } | |
| template<typename T> | |
| auto address_of_type_as_const_void_ptr() | |
| { | |
| return [](T const& arg) mutable -> void const* { return &arg; }; | |
| } |
🤖 Prompt for AI Agents
In src/core/utilities/variants.hpp around lines 11 to 15, the
`address_of_type_as_void_ptr` function uses an unsafe `const_cast` to convert
the input argument to a void pointer. This can lead to undefined behavior if the
input argument is a const object, violating const-correctness and potentially
causing subtle bugs or crashes. To fix this issue, provide separate overloads
for const and non-const input arguments, allowing the compiler to select the
appropriate version based on the const-ness of the input. This will ensure the
function behaves correctly and maintains the expected const-correctness.
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.
| if (start == variants.size()) | ||
| std::runtime_error("Required name not found in variants: " + arg.name()); |
There was a problem hiding this comment.
Missing throw statement causes undefined behavior.
The function constructs an error message but never throws it, leading to undefined behavior when the required name is not found.
if (start == variants.size())
- std::runtime_error("Required name not found in variants: " + arg.name());
+ throw std::runtime_error("Required name not found in variants: " + arg.name());📝 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.
| 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()); |
🤖 Prompt for AI Agents
In src/core/utilities/variants.hpp at lines 116 to 117, the code constructs a
std::runtime_error but does not throw it, causing undefined behavior. Fix this
by adding the 'throw' keyword before the std::runtime_error construction to
properly throw the exception when the required name is not found.
| auto& rho = get_as_ref_or_throw<FieldResource>(resources); | ||
| } | ||
| { | ||
| auto& B = get_as_ref_or_throw<VecFieldResource>(resources); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
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.
roch is using this on his close branch so we might aswell get it in separately
allow conditional runtime resource allocation via variants