suggestive constexpr tuple validNbrParticles #972
Conversation
📝 WalkthroughWalkthroughThis pull request expands the testing framework and enhances utility functionality. A new subdirectory is added in the CMake configuration so that tests in the meta utilities module are built. In the meta utilities header, several functions are introduced for working with simulator configurations and a minor adjustment in const qualifier ordering is made. The types header is updated to accept extra arguments in the template function for_N and refines a pointer’s const annotation in get_env. Additionally, a dedicated test project (with new CMakeLists.txt and unit test source) is added to validate the new meta utilities functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant FT as MetaUtilitiesTest
participant MU as Meta Utilities (validNbrParticlesFor)
TR->>FT: Start test "testvalidNbrParticlesFor"
FT->>MU: Invoke validNbrParticlesFor with 2D/2 interpolation parameters
MU-->>FT: Return computed number of valid particles
FT->>TR: Assert returned value equals expected (5)
✨ 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:
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
|
| @@ -0,0 +1,30 @@ | |||
| #include <string> | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/core/utilities/meta/test_meta_utilities.cpp (2)
11-13: Consider enhancing the test fixture.The test fixture
MetaUtilitiesTestis declared but doesn't contain any setup or teardown code. Either add common setup/teardown operations or use TEST directly without a fixture if no shared context is needed.
15-23: Test implementation looks good, consider adding more test cases.The test correctly validates the
validNbrParticlesForfunction for a 2D space with 2 interpolation points. However, it would be valuable to add more test cases with different dimensions and interpolation points to ensure comprehensive coverage.TEST(MetaUtilitiesTest, testvalidNbrParticlesFor) { std::size_t constexpr static dim = 2; std::size_t constexpr static interp = 2; auto constexpr validNbrParticlesTuple = validNbrParticlesFor<dim, interp>(); auto constexpr n_validNbrParticles = std::tuple_size_v<decltype(validNbrParticlesTuple)>; EXPECT_EQ(n_validNbrParticles, 5); + + // Test cases for different dimensions and interpolation points + auto constexpr validNbrParticlesTuple1D = validNbrParticlesFor<1, interp>(); + auto constexpr n_validNbrParticles1D = std::tuple_size_v<decltype(validNbrParticlesTuple1D)>; + EXPECT_GT(n_validNbrParticles1D, 0); + + auto constexpr validNbrParticlesTuple3D = validNbrParticlesFor<3, interp>(); + auto constexpr n_validNbrParticles3D = std::tuple_size_v<decltype(validNbrParticlesTuple3D)>; + EXPECT_GT(n_validNbrParticles3D, 0); }src/core/utilities/meta/meta_utilities.hpp (2)
91-96: Consider returning std::array instead of tuple for easier iterationThe function correctly extracts NbRefinedParts from the simulator tuple. However, returning std::array instead of tuple would allow easier iteration over the valid particle numbers at runtime.
template<typename Dimension, typename InterpOrder, typename... NbRefinedParts> auto static constexpr validNbrParticlesFor( std::tuple<Dimension, InterpOrder, NbRefinedParts...> const&) { - return std::tuple<NbRefinedParts...>{}; + return std::array<size_t, sizeof...(NbRefinedParts)>{NbRefinedParts::value...}; }
117-122: Add static_assert to validate template parametersThe function provides a convenient way to retrieve valid particle numbers, but it should validate the dim and interp parameters at compile time to provide better error messages.
template<std::size_t dim, std::size_t interp> auto static constexpr validNbrParticlesFor() { + static_assert(dim >= 1 && dim <= 3, "Dimension must be between 1 and 3"); + static_assert(interp >= 1 && interp <= 3, "Interpolation order must be between 1 and 3"); return validNbrParticlesFor( std::get<simulatorTupleIndex<dim, interp>()>(possibleSimulators())); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
res/cmake/test.cmake(1 hunks)src/core/utilities/meta/meta_utilities.hpp(5 hunks)src/core/utilities/types.hpp(3 hunks)tests/core/utilities/meta/CMakeLists.txt(1 hunks)tests/core/utilities/meta/test_meta_utilities.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative t...
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/utilities/meta/meta_utilities.hppsrc/core/utilities/types.hpp
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (13)
res/cmake/test.cmake (1)
24-24: Addition of meta utilities tests looks good.The new test directory for meta utilities is properly added to the CMake configuration, following the same pattern as the other test directories.
tests/core/utilities/meta/test_meta_utilities.cpp (2)
1-10: Appropriate includes and namespace usage.The includes are properly organized - standard library headers first, followed by project-specific headers, and then testing framework headers. The using directive for the PHARE::core namespace makes the code more readable.
25-30: Main function implementation is correct.The main function correctly initializes Google Test and runs all tests.
tests/core/utilities/meta/CMakeLists.txt (1)
1-18: CMake configuration looks good.The CMake configuration properly sets up the project, adds the executable with the correct source file, includes Google Test, and links against the required libraries. The test is configured correctly with
add_no_mpi_phare_test.src/core/utilities/types.hpp (6)
229-229: Consistent const ordering in pointer type.Changing from
const char*tochar const*follows a more consistent pattern for const qualification in the codebase, with the const qualifier placed to the right of the type. Both are semantically identical, but this change improves consistency.
427-429: Enhanced flexibility with variadic template parameters.The function template has been improved to accept additional arguments that will be forwarded to the callable function. This change enhances the flexibility and reusability of the
for_Nfunction.
437-440: Updated return type calculation to account for variadic arguments.The return type calculation has been correctly updated to include the variadic arguments in the function invocation. This ensures proper type deduction when the function is called with additional arguments.
446-452: Forward additional arguments in tuple creation.The implementation correctly passes the additional arguments to the callable function in all three return modes (make_tuple, make_array, forward_tuple). This ensures consistent behavior across all return types.
458-458: Forward arguments in non-returning function call.The implementation correctly passes the additional arguments to the callable function when it doesn't return a value. This ensures consistent behavior between returning and non-returning callables.
461-464: Updated second overload for consistency.The second overload of
for_Nhas been properly updated to match the first overload's signature, accepting and forwarding variadic template parameters. This ensures both overloads provide the same functionality with different callable types.src/core/utilities/meta/meta_utilities.hpp (3)
21-21: Good use of conventional const placementChanging from
static const type value = 0;tostatic type const value = 0;follows the C++ convention of placingconstafter the type for constants, making the code more idiomatic.
86-89: LGTM!This well-crafted utility function leverages
std::tuple_size_vto determine the number of possible simulators at compile time. The constexpr specifier ensures compile-time evaluation.
132-133: Good use of type alias templateUsing
std::tuple_element_t<0, SimuType>instead oftypename std::tuple_element<0, SimuType>::typeimproves code readability while maintaining identical functionality.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn> | ||
| constexpr auto for_N(Fn& fn) | ||
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn, typename... Args> | ||
| constexpr auto for_N(Fn& fn, Args&&... args) |
Check notice
Code scanning / CodeQL
Unused static variable Note
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn> | ||
| constexpr auto for_N(Fn& fn) | ||
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn, typename... Args> | ||
| constexpr auto for_N(Fn& fn, Args&&... args) |
Check notice
Code scanning / CodeQL
Unused local variable Note
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn> | ||
| constexpr auto for_N(Fn&& fn) | ||
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn, typename... Args> | ||
| constexpr auto for_N(Fn&& fn, Args&&... args) |
Check notice
Code scanning / CodeQL
Unused static variable Note
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn> | ||
| constexpr auto for_N(Fn&& fn) | ||
| template<std::uint16_t N, auto M = for_N_R_mode::make_tuple, typename Fn, typename... Args> | ||
| constexpr auto for_N(Fn&& fn, Args&&... args) |
Check notice
Code scanning / CodeQL
Unused local variable Note
|
|
||
| TEST(MetaUtilitiesTest, testvalidNbrParticlesFor) | ||
| { | ||
| std::size_t constexpr static dim = 2; |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| TEST(MetaUtilitiesTest, testvalidNbrParticlesFor) | ||
| { | ||
| std::size_t constexpr static dim = 2; | ||
| std::size_t constexpr static interp = 2; |
Check notice
Code scanning / CodeQL
Unused local variable Note test
getting the tuple/set of possible refined particle count per dim/interp for @UCaromel mostly