From c89aa70668c6446982095f13bfcc4b14f8d56359 Mon Sep 17 00:00:00 2001 From: Matthias Schimek Date: Mon, 8 Jan 2024 10:06:22 +0100 Subject: [PATCH] extract() <=> data buffer is owning (#610) * extract is now possible for owning data buffers * update docu * add ampersand and white space * include specialization in internal namespace * fix clang error --- include/kamping/data_buffer.hpp | 5 +++- include/kamping/result.hpp | 53 ++++++++++++++++----------------- tests/data_buffer_test.cpp | 29 ++++++++++++------ tests/named_parameters_test.cpp | 12 ++++---- tests/result_test.cpp | 10 +++---- 5 files changed, 60 insertions(+), 49 deletions(-) diff --git a/include/kamping/data_buffer.hpp b/include/kamping/data_buffer.hpp index 597c7da84..b1766cc99 100644 --- a/include/kamping/data_buffer.hpp +++ b/include/kamping/data_buffer.hpp @@ -306,6 +306,9 @@ class DataBuffer : private ParameterObjectBase { /// @brief Indicates whether the buffer is allocated by KaMPIng. static constexpr bool is_lib_allocated = allocation == BufferAllocation::lib_allocated; + static constexpr bool is_owning = + ownership == BufferOwnership::owning; ///< Indicates whether the buffer owns its underlying storage. + static constexpr bool is_modifiable = modifiability == BufferModifiability::modifiable; ///< Indicates whether the underlying storage is modifiable. static constexpr bool is_single_element = @@ -502,7 +505,7 @@ class DataBuffer : private ParameterObjectBase { /// state. /// /// @return Moves the underlying container out of the DataBuffer. - template = true> + template = true> MemberTypeWithConst extract() { static_assert( ownership == BufferOwnership::owning, diff --git a/include/kamping/result.hpp b/include/kamping/result.hpp index 7b8da27c6..42b421231 100644 --- a/include/kamping/result.hpp +++ b/include/kamping/result.hpp @@ -1,6 +1,6 @@ // This file is part of KaMPIng. // -// Copyright 2021-2023 The KaMPIng Authors +// Copyright 2021-2024 The KaMPIng Authors // // KaMPIng is free software : you can redistribute it and/or modify it under the terms of the GNU Lesser General Public // License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later @@ -36,6 +36,15 @@ inline constexpr bool has_extract_v = has_member_extract_v; /// @brief Use this type if one of the template parameters of MPIResult is not used for a specific wrapped \c MPI call. struct ResultCategoryNotUsed {}; + +/// @brief Helper for implementing the extract_* functions in \ref MPIResult. Is \c true if the passed buffer type owns +/// its underlying storage and is an output buffer. +template +inline constexpr bool is_extractable = Buffer::is_owning&& Buffer::is_out_buffer; + +/// @brief Specialization of helper for implementing the extract_* functions in \ref MPIResult. Is always \c false; +template <> +inline constexpr bool is_extractable = false; } // namespace internal /// @brief MPIResult contains the result of a \c MPI call wrapped by KaMPIng. @@ -129,7 +138,7 @@ class MPIResult { /// /// This function is only available if the underlying status is owned by the /// MPIResult object. - /// @tparam StatusType_ Template parameter helper only needed to remove this + /// @tparam StatusObject_ Template parameter helper only needed to remove this /// function if StatusType does not possess a member function \c extract(). /// @return Returns the underlying status object. template < @@ -144,9 +153,9 @@ class MPIResult { /// This function is only available if the underlying memory is owned by the /// MPIResult object. /// @tparam RecvBuf_ Template parameter helper only needed to remove this - /// function if RecvBuf does not possess a member function \c extract(). + /// function if RecvBuf should not be extracted (it does not own its underlying memory or is not an out-buffer). /// @return Returns the underlying storage containing the received elements. - template , bool> = true> + template , bool> = true> decltype(auto) extract_recv_buffer() { return _recv_buffer.extract(); } @@ -157,9 +166,7 @@ class MPIResult { /// @tparam RecvCounts_ Template parameter helper only needed to remove this function if RecvCounts does not possess /// a member function \c extract(). /// @return Returns the underlying storage containing the receive counts. - template < - typename RecvCounts_ = RecvCounts, - std::enable_if_t, bool> = true> + template , bool> = true> decltype(auto) extract_recv_counts() { return _recv_counts.extract(); } @@ -170,9 +177,7 @@ class MPIResult { /// @tparam RecvCount_ Template parameter helper only needed to remove this function if RecvCount does not /// possess a member function \c extract(). /// @return Returns the underlying storage containing the recv count. - template < - typename RecvCount_ = RecvCount, - std::enable_if_t, bool> = true> + template , bool> = true> decltype(auto) extract_recv_count() { return _recv_count.extract(); } @@ -183,9 +188,7 @@ class MPIResult { /// @tparam RecvDispls_ Template parameter helper only needed to remove this function if RecvDispls does not possess /// a member function \c extract(). /// @return Returns the underlying storage containing the receive displacements. - template < - typename RecvDispls_ = RecvDispls, - std::enable_if_t, bool> = true> + template , bool> = true> decltype(auto) extract_recv_displs() { return _recv_displs.extract(); } @@ -196,9 +199,7 @@ class MPIResult { /// @tparam SendCounts_ Template parameter helper only needed to remove this function if SendCounts does not possess /// a member function \c extract(). /// @return Returns the underlying storage containing the send counts. - template < - typename SendCounts_ = SendCounts, - std::enable_if_t, bool> = true> + template , bool> = true> decltype(auto) extract_send_counts() { return _send_counts.extract(); } @@ -209,9 +210,7 @@ class MPIResult { /// @tparam SendCount_ Template parameter helper only needed to remove this function if SendCount does not /// possess a member function \c extract(). /// @return Returns the underlying storage containing the send count. - template < - typename SendCount_ = SendCount, - std::enable_if_t, bool> = true> + template , bool> = true> decltype(auto) extract_send_count() { return _send_count.extract(); } @@ -222,9 +221,7 @@ class MPIResult { /// @tparam SendDispls_ Template parameter helper only needed to remove this function if SendDispls does not possess /// a member function \c extract(). /// @return Returns the underlying storage containing the send displacements. - template < - typename SendDispls_ = SendDispls, - std::enable_if_t, bool> = true> + template , bool> = true> decltype(auto) extract_send_displs() { return _send_displs.extract(); } @@ -236,8 +233,8 @@ class MPIResult { /// possess a member function \c extract(). /// @return Returns the underlying storage containing the send_recv_count. template < - typename SendRecvCount_ = SendRecvCount, - std::enable_if_t, bool> = true> + typename SendRecvCount_ = SendRecvCount, + std::enable_if_t, bool> = true> decltype(auto) extract_send_recv_count() { return _send_recv_count.extract(); } @@ -248,7 +245,7 @@ class MPIResult { /// @tparam SendType_ Template parameter helper only needed to remove this function if SendType does not /// possess a member function \c extract(). /// @return Returns the underlying storage containing the send_type. - template , bool> = true> + template , bool> = true> decltype(auto) extract_send_type() { return _send_type.extract(); } @@ -259,7 +256,7 @@ class MPIResult { /// @tparam RecvType_ Template parameter helper only needed to remove this function if RecvType does not /// possess a member function \c extract(). /// @return Returns the underlying storage containing the send_type. - template , bool> = true> + template , bool> = true> decltype(auto) extract_recv_type() { return _recv_type.extract(); } @@ -270,8 +267,8 @@ class MPIResult { /// possess a member function \c extract(). /// @return Returns the underlying storage containing the send_type. template < - typename SendRecvType_ = SendRecvType, - std::enable_if_t, bool> = true> + typename SendRecvType_ = SendRecvType, + std::enable_if_t, bool> = true> decltype(auto) extract_send_recv_type() { return _send_recv_type.extract(); } diff --git a/tests/data_buffer_test.cpp b/tests/data_buffer_test.cpp index a8ad473ae..754dfe4c5 100644 --- a/tests/data_buffer_test.cpp +++ b/tests/data_buffer_test.cpp @@ -591,7 +591,7 @@ TEST(DataBufferTest, has_extract) { "Library allocated DataBuffers must have an extract() member function" ); static_assert( - !has_extract_v>, - "User allocated DataBuffers must not have an extract() member function" + "User allocated owning DataBuffers must have an extract() member function" + ); + static_assert( + !has_extract_v>, + "User allocated referencing DataBuffers must not have an extract() member function" ); } @@ -711,14 +722,14 @@ TEST(LibAllocatedContainerBasedBufferTest, prevent_usage_after_extraction) { } TEST(LibAllocatedContainerBasedBufferTest, prevent_usage_after_extraction_via_mpi_result) { - LibAllocatedContainerBasedBuffer, ParameterType::recv_buf, BufferType::in_buffer> recv_buffer; - LibAllocatedContainerBasedBuffer, ParameterType::recv_counts, BufferType::in_buffer> recv_counts; - LibAllocatedContainerBasedBuffer, ParameterType::recv_displs, BufferType::in_buffer> recv_displs; - LibAllocatedContainerBasedBuffer, ParameterType::send_counts, BufferType::in_buffer> send_counts; - LibAllocatedContainerBasedBuffer, ParameterType::send_displs, BufferType::in_buffer> send_displs; + LibAllocatedContainerBasedBuffer, ParameterType::recv_buf, BufferType::out_buffer> recv_buffer; + LibAllocatedContainerBasedBuffer, ParameterType::recv_counts, BufferType::out_buffer> recv_counts; + LibAllocatedContainerBasedBuffer, ParameterType::recv_displs, BufferType::out_buffer> recv_displs; + LibAllocatedContainerBasedBuffer, ParameterType::send_counts, BufferType::out_buffer> send_counts; + LibAllocatedContainerBasedBuffer, ParameterType::send_displs, BufferType::out_buffer> send_displs; // we use out_buffer here because extracting is only done from out buffers - LibAllocatedContainerBasedBuffer recv_count; - LibAllocatedContainerBasedBuffer send_count; + LibAllocatedContainerBasedBuffer recv_count; + LibAllocatedContainerBasedBuffer send_count; LibAllocatedContainerBasedBuffer send_recv_count; LibAllocatedContainerBasedBuffer send_type; LibAllocatedContainerBasedBuffer recv_type; diff --git a/tests/named_parameters_test.cpp b/tests/named_parameters_test.cpp index c978bfa15..f547a97a0 100644 --- a/tests/named_parameters_test.cpp +++ b/tests/named_parameters_test.cpp @@ -1495,7 +1495,7 @@ TEST(ParameterFactoriesTest, make_data_buffer) { "Owning buffers must hold their data directly." ); // extract() as proxy for lib allocated DataBuffers - EXPECT_FALSE(has_extract_v); + EXPECT_TRUE(has_extract_v); } { @@ -1552,7 +1552,7 @@ TEST(ParameterFactoriesTest, make_data_buffer) { "Owning buffers must hold their data directly." ); // extract() as proxy for lib allocated DataBuffers - EXPECT_FALSE(has_extract_v); + EXPECT_TRUE(has_extract_v); } { // Constant, container, owning, user_allocated with initializer_list @@ -1570,7 +1570,7 @@ TEST(ParameterFactoriesTest, make_data_buffer) { "Owning buffers must hold their data directly." ); // extract() as proxy for lib allocated DataBuffers - EXPECT_FALSE(has_extract_v); + EXPECT_TRUE(has_extract_v); } } @@ -1656,7 +1656,7 @@ TEST(ParameterFactoriesTest, make_data_buffer_boolean_value) { "Owning buffers must hold their data directly." ); // extract() as proxy for lib allocated DataBuffers - EXPECT_FALSE(has_extract_v); + EXPECT_TRUE(has_extract_v); } { @@ -1714,7 +1714,7 @@ TEST(ParameterFactoriesTest, make_data_buffer_boolean_value) { "Initializer lists of type bool have to be converted to std::vector." ); // extract() as proxy for lib allocated DataBuffers - EXPECT_FALSE(has_extract_v); + EXPECT_TRUE(has_extract_v); } { // Constant, container, owning, user_allocated with initializer_list @@ -1733,7 +1733,7 @@ TEST(ParameterFactoriesTest, make_data_buffer_boolean_value) { "Initializer lists of type bool have to be converted to std::vector." ); // extract() as proxy for lib allocated DataBuffers - EXPECT_FALSE(has_extract_v); + EXPECT_TRUE(has_extract_v); } } diff --git a/tests/result_test.cpp b/tests/result_test.cpp index db246f422..906c7b6f8 100644 --- a/tests/result_test.cpp +++ b/tests/result_test.cpp @@ -101,7 +101,7 @@ void test_recv_count_in_MPIResult() { using namespace kamping; using namespace kamping::internal; - LibAllocatedSingleElementBuffer recv_count_wrapper{}; + LibAllocatedSingleElementBuffer recv_count_wrapper{}; recv_count_wrapper.underlying() = 42; MPIResult mpi_result{ ResultCategoryNotUsed{}, @@ -415,7 +415,7 @@ KAMPING_MAKE_HAS_MEMBER(extract_send_recv_type) TEST(MpiResultTest, removed_extract_functions) { using namespace ::kamping; using namespace ::kamping::internal; - constexpr BufferType btype = BufferType::in_buffer; + constexpr BufferType btype = BufferType::out_buffer; { // All of these should be extractable (used to make sure that the above macros work correctly) StatusParam status_sanity_check; @@ -642,7 +642,7 @@ TEST(MpiResultTest, removed_extract_functions) { TEST(MakeMpiResultTest, pass_random_order_buffer) { { - constexpr BufferType btype = BufferType::in_buffer; + constexpr BufferType btype = BufferType::out_buffer; LibAllocatedContainerBasedBuffer, ParameterType::recv_counts, btype> recv_counts; LibAllocatedContainerBasedBuffer, ParameterType::recv_buf, btype> recv_buf; LibAllocatedContainerBasedBuffer, ParameterType::recv_displs, btype> recv_displs; @@ -663,7 +663,7 @@ TEST(MakeMpiResultTest, pass_random_order_buffer) { ASSERT_EQ(result_status.tag(), 42); } { - constexpr BufferType btype = BufferType::in_buffer; + constexpr BufferType btype = BufferType::out_buffer; LibAllocatedContainerBasedBuffer, ParameterType::recv_counts, btype> recv_counts; LibAllocatedContainerBasedBuffer, ParameterType::recv_buf, btype> recv_buf; @@ -686,7 +686,7 @@ TEST(MakeMpiResultTest, pass_send_recv_buf) { } TEST(MakeMpiResultTest, check_content) { - constexpr BufferType btype = BufferType::in_buffer; + constexpr BufferType btype = BufferType::out_buffer; std::vector recv_buf_data(20); std::iota(recv_buf_data.begin(), recv_buf_data.end(), 0);