From a6969277d4d16169227adeea88f3df03b7a0399a Mon Sep 17 00:00:00 2001 From: Matthias Schimek Date: Mon, 30 Sep 2024 08:40:32 +0200 Subject: [PATCH] Fix/do not store owning data as const (#752) * In const owning data buffers: do not store the data as consts as this enforces copying the data once it is moved out * fix formatting * fix compile error * remove accidentaly made edit * fix test * add check for vector --- include/kamping/data_buffer.hpp | 22 ++++- tests/named_parameters_test.cpp | 140 ++++++++++++++++++++++++------- tests/parameter_objects_test.cpp | 21 +++++ 3 files changed, 149 insertions(+), 34 deletions(-) diff --git a/include/kamping/data_buffer.hpp b/include/kamping/data_buffer.hpp index a81840434..1f83fa4a1 100644 --- a/include/kamping/data_buffer.hpp +++ b/include/kamping/data_buffer.hpp @@ -370,6 +370,8 @@ template < typename ValueType = default_value_type_tag> class DataBuffer : private CopyMoveEnabler>, private Extractable { public: + static_assert(!std::is_const_v, "Member Type should not be const qualified."); + static constexpr TParameterType parameter_type = parameter_type_param; ///< The type of parameter this buffer represents. @@ -411,6 +413,16 @@ class DataBuffer : private CopyMoveEnabler MemberTypeWithConst, MemberTypeWithConst&>; ///< The ContainerType as const or non-const (see ContainerTypeWithConst) and ///< reference or non-reference depending on ownership. + /// + using StorageType = std::conditional_t< + is_owning, + MemberType, + MemberTypeWithConstAndRef>; ///< The type as which the underlying container will be stored. If the buffer is + ///< owning, i.e. the underlying data is not referenced but stored directly, the + ///< potential constness of the data is not reflected in StorageType as this would + ///< enforce copying of the \c const data once it will be extracted. Modifying const + ///< data is instead prevented by giving only const qualified access via + ///< underlying() or data() in such case. using value_type = typename ValueTypeWrapper::value_type; ///< Value type of the buffer. @@ -586,21 +598,25 @@ class DataBuffer : private CopyMoveEnabler /// /// @return Moves the underlying container out of the DataBuffer. template = true> - MemberTypeWithConst extract() { + StorageType extract() { static_assert( ownership == BufferOwnership::owning, "Moving out of a reference should not be done because it would leave " "a users container in an unspecified state." ); + static_assert( + !is_vector_bool_v, + "Buffers based on std::vector are not supported, use std::vector instead." + ); kassert_not_extracted("Cannot extract a buffer that has already been extracted."); - auto extracted = std::move(underlying()); + auto extracted = std::move(_data); // we set is_extracted here because otherwise the call to underlying() would fail set_extracted(); return extracted; } private: - MemberTypeWithConstAndRef _data; ///< Container which holds the actual data. + StorageType _data; ///< Container which holds the actual data. }; /// @brief A more generic version of a DataBuffer which stores an object of type \tparam MemberType with its associcated diff --git a/tests/named_parameters_test.cpp b/tests/named_parameters_test.cpp index 8aa9e41fb..2ab884e40 100644 --- a/tests/named_parameters_test.cpp +++ b/tests/named_parameters_test.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -30,8 +31,8 @@ using namespace ::kamping::internal; namespace testing { template -void test_const_buffer( - GeneratedBuffer const& generated_buffer, +void test_const_referencing_buffer( + GeneratedBuffer& generated_buffer, kamping::internal::ParameterType expected_parameter_type, kamping::internal::BufferType expected_buffer_type, Span& expected_span @@ -40,9 +41,19 @@ void test_const_buffer( static_assert(std::is_same_v); EXPECT_FALSE(GeneratedBuffer::is_modifiable); + EXPECT_FALSE(GeneratedBuffer::is_owning); EXPECT_EQ(GeneratedBuffer::parameter_type, expected_parameter_type); EXPECT_EQ(GeneratedBuffer::buffer_type, expected_buffer_type); + static_assert( + std::is_const_v>, + "Member data() of the generated buffer does not point to const memory." + ); + static_assert( + std::is_const_v>, + "Member underlying() of the generated buffer provides access to non-const memory." + ); + auto span = generated_buffer.get(); static_assert(std::is_pointer_v, "Member ptr of internal::Span is not a pointer."); static_assert( @@ -59,8 +70,8 @@ void test_const_buffer( } template -void test_owning_buffer( - GeneratedBuffer const& generated_buffer, +void test_const_owning_buffer( + GeneratedBuffer& generated_buffer, kamping::internal::ParameterType expected_parameter_type, kamping::internal::BufferType expected_buffer_type, ExpectedValueContainer&& expected_value_container @@ -69,9 +80,19 @@ void test_owning_buffer( static_assert(std::is_same_v); EXPECT_FALSE(GeneratedBuffer::is_modifiable); + EXPECT_TRUE(GeneratedBuffer::is_owning); EXPECT_EQ(GeneratedBuffer::parameter_type, expected_parameter_type); EXPECT_EQ(GeneratedBuffer::buffer_type, expected_buffer_type); + static_assert( + std::is_const_v>, + "Member data() of the generated buffer does not point to const memory." + ); + static_assert( + std::is_const_v>, + "Member underlying() of the generated buffer provides access to non-const memory." + ); + auto span = generated_buffer.get(); static_assert(std::is_pointer_v, "Member ptr of internal::Span is not a pointer."); static_assert( @@ -205,7 +226,7 @@ TEST(ParameterFactoriesTest, send_buf_basics_int_vector) { auto gen_via_int_vec = send_buf(int_vec).construct_buffer_or_rebind(); Span expected_span{int_vec.data(), int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_int_vec, ParameterType::send_buf, internal::BufferType::in_buffer, @@ -218,7 +239,7 @@ TEST(ParameterFactoriesTest, send_buf_basics_const_int_vector) { auto gen_via_const_int_vec = send_buf(const_int_vec).construct_buffer_or_rebind(); Span expected_span{const_int_vec.data(), const_int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_const_int_vec, ParameterType::send_buf, internal::BufferType::in_buffer, @@ -231,7 +252,7 @@ TEST(ParameterFactoriesTest, send_buf_basics_moved_vector) { std::vector const expected = const_int_vec; auto gen_via_moved_vec = send_buf(std::move(const_int_vec)).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_moved_vec, ParameterType::send_buf, internal::BufferType::in_buffer, @@ -247,7 +268,7 @@ TEST(ParameterFactoriesTest, send_buf_basics_vector_from_function) { std::vector const expected = make_vector(); auto gen_via_vec_from_function = send_buf(make_vector()).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_vec_from_function, ParameterType::send_buf, internal::BufferType::in_buffer, @@ -259,7 +280,7 @@ TEST(ParameterFactoriesTest, send_buf_basics_vector_from_initializer_list) { std::vector expected = {1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_vec_from_function = send_buf({1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_vec_from_function, ParameterType::send_buf, internal::BufferType::in_buffer, @@ -384,12 +405,31 @@ TEST(ParameterFactoriesTest, send_buf_ignored) { EXPECT_EQ(ignored_send_buf.get().size(), 0); } +TEST(ParameterFactoriesTest, send_buf_owning_move_only_data) { + // test that data within the buffer is still treated as constant but can be returned without being copied + testing::NonCopyableOwnContainer vec{1, 2, 3, 4}; // required as original data will be moved to buffer + testing::NonCopyableOwnContainer const expected_vec{ + 1, + 2, + 3, + 4}; // required as original data will be moved to buffer + auto send_buffer = send_buf(std::move(vec)).construct_buffer_or_rebind(); + testing::test_const_owning_buffer( + send_buffer, + ParameterType::send_buf, + internal::BufferType::in_buffer, + expected_vec + ); + auto extracted_vec = send_buffer.extract(); + EXPECT_THAT(extracted_vec, testing::ElementsAre(1, 2, 3, 4)); +} + TEST(ParameterFactoriesTest, send_counts_basics_int_vector) { std::vector int_vec{1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_int_vec = send_counts(int_vec).construct_buffer_or_rebind(); Span expected_span{int_vec.data(), int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_int_vec, ParameterType::send_counts, internal::BufferType::in_buffer, @@ -402,7 +442,7 @@ TEST(ParameterFactoriesTest, send_counts_basics_const_int_vector) { auto gen_via_const_int_vec = send_counts(const_int_vec).construct_buffer_or_rebind(); Span expected_span{const_int_vec.data(), const_int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_const_int_vec, ParameterType::send_counts, internal::BufferType::in_buffer, @@ -416,7 +456,7 @@ TEST(ParameterFactoriesTest, send_counts_basics_moved_int_vector) { auto gen_via_int_vec = send_counts(std::move(int_vec)).construct_buffer_or_rebind(); Span expected_span{int_vec.data(), int_vec.size()}; using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_int_vec, ParameterType::send_counts, internal::BufferType::in_buffer, @@ -428,7 +468,7 @@ TEST(ParameterFactoriesTest, send_counts_basics_initializer_list) { std::vector expected{1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_int_initializer_list = send_counts({1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_int_initializer_list, ParameterType::send_counts, internal::BufferType::in_buffer, @@ -436,12 +476,31 @@ TEST(ParameterFactoriesTest, send_counts_basics_initializer_list) { ); } +TEST(ParameterFactoriesTest, send_counts_owning_move_only_data) { + // test that data within the buffer is still treated as constant but can be returned without being copied + testing::NonCopyableOwnContainer vec{1, 2, 3, 4}; // required as original data will be moved to buffer + testing::NonCopyableOwnContainer const expected_vec{ + 1, + 2, + 3, + 4}; // required as original data will be moved to buffer + auto send_buffer = send_counts(std::move(vec)).construct_buffer_or_rebind(); + testing::test_const_owning_buffer( + send_buffer, + ParameterType::send_counts, + internal::BufferType::in_buffer, + expected_vec + ); + auto extracted_vec = send_buffer.extract(); + EXPECT_THAT(extracted_vec, testing::ElementsAre(1, 2, 3, 4)); +} + TEST(ParameterFactoriesTest, recv_counts_in_basics_int_vector) { std::vector int_vec{1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_int_vec = recv_counts(int_vec).construct_buffer_or_rebind(); Span expected_span{int_vec.data(), int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_int_vec, ParameterType::recv_counts, internal::BufferType::in_buffer, @@ -454,7 +513,7 @@ TEST(ParameterFactoriesTest, recv_counts_in_basics_const_int_vector) { auto gen_via_const_int_vec = recv_counts(const_int_vec).construct_buffer_or_rebind(); Span expected_span{const_int_vec.data(), const_int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_const_int_vec, ParameterType::recv_counts, internal::BufferType::in_buffer, @@ -467,7 +526,7 @@ TEST(ParameterFactoriesTest, recv_counts_in_basics_moved_vector) { auto expected = int_vec; auto gen_via_moved_vec = recv_counts(std::move(int_vec)).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_moved_vec, ParameterType::recv_counts, internal::BufferType::in_buffer, @@ -479,7 +538,7 @@ TEST(ParameterFactoriesTest, recv_counts_in_basics_initializer_list) { std::vector expected{1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_initializer_list = recv_counts({1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_initializer_list, ParameterType::recv_counts, internal::BufferType::in_buffer, @@ -492,7 +551,7 @@ TEST(ParameterFactoriesTest, send_displs_in_basics_int_vector) { auto gen_via_int_vec = send_displs(int_vec).construct_buffer_or_rebind(); Span expected_span{int_vec.data(), int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_int_vec, ParameterType::send_displs, internal::BufferType::in_buffer, @@ -505,7 +564,7 @@ TEST(ParameterFactoriesTest, send_displs_in_basics_const_int_vector) { auto gen_via_const_int_vec = send_displs(const_int_vec).construct_buffer_or_rebind(); Span expected_span{const_int_vec.data(), const_int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_const_int_vec, ParameterType::send_displs, internal::BufferType::in_buffer, @@ -518,7 +577,7 @@ TEST(ParameterFactoriesTest, send_displs_in_basics_moved_vector) { auto expected = int_vec; auto gen_via_moved_vec = send_displs(std::move(int_vec)).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_moved_vec, ParameterType::send_displs, internal::BufferType::in_buffer, @@ -530,7 +589,7 @@ TEST(ParameterFactoriesTest, send_displs_in_basics_initializer_list) { std::vector expected{1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_initializer_list = send_displs({1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_initializer_list, ParameterType::send_displs, internal::BufferType::in_buffer, @@ -538,12 +597,31 @@ TEST(ParameterFactoriesTest, send_displs_in_basics_initializer_list) { ); } +TEST(ParameterFactoriesTest, send_displs_owning_move_only_data) { + // test that data within the buffer is still treated as constant but can be returned without being copied + testing::NonCopyableOwnContainer vec{1, 2, 3, 4}; // required as original data will be moved to buffer + testing::NonCopyableOwnContainer const expected_vec{ + 1, + 2, + 3, + 4}; // required as original data will be moved to buffer + auto send_buffer = send_displs(std::move(vec)).construct_buffer_or_rebind(); + testing::test_const_owning_buffer( + send_buffer, + ParameterType::send_displs, + internal::BufferType::in_buffer, + expected_vec + ); + auto extracted_vec = send_buffer.extract(); + EXPECT_THAT(extracted_vec, testing::ElementsAre(1, 2, 3, 4)); +} + TEST(ParameterFactoriesTest, recv_displs_in_basics_int_vector) { std::vector int_vec{1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_int_vec = recv_displs(int_vec).construct_buffer_or_rebind(); Span expected_span{int_vec.data(), int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_int_vec, ParameterType::recv_displs, internal::BufferType::in_buffer, @@ -556,7 +634,7 @@ TEST(ParameterFactoriesTest, recv_displs_in_basics_const_int_vector) { auto gen_via_const_int_vec = recv_displs(const_int_vec).construct_buffer_or_rebind(); Span expected_span{const_int_vec.data(), const_int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_const_int_vec, ParameterType::recv_displs, internal::BufferType::in_buffer, @@ -569,7 +647,7 @@ TEST(ParameterFactoriesTest, recv_displs_in_basics_moved_vector) { auto expected = int_vec; auto gen_via_moved_vec = recv_displs(std::move(int_vec)).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_moved_vec, ParameterType::recv_displs, internal::BufferType::in_buffer, @@ -581,7 +659,7 @@ TEST(ParameterFactoriesTest, recv_displs_in_basics_initializer_list) { std::vector expected{1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_initializer_list = recv_displs({1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_initializer_list, ParameterType::recv_displs, internal::BufferType::in_buffer, @@ -1218,7 +1296,7 @@ TEST(ParameterFactoriesTest, send_recv_buf_basics_const_int_vector) { auto gen_via_const_int_vec = send_recv_buf(const_int_vec).construct_buffer_or_rebind(); Span expected_span{const_int_vec.data(), const_int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_const_int_vec, ParameterType::send_recv_buf, internal::BufferType::in_out_buffer, @@ -1514,7 +1592,7 @@ TEST(ParameterFactoriesTest, values_on_rank_0_basics_int_vector) { auto gen_via_int_vec = values_on_rank_0(int_vec).construct_buffer_or_rebind(); Span expected_span{int_vec.data(), int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_int_vec, ParameterType::values_on_rank_0, BufferType::in_buffer, @@ -1527,7 +1605,7 @@ TEST(ParameterFactoriesTest, values_on_rank_0_basics_const_int_vector) { auto gen_via_const_int_vec = values_on_rank_0(const_int_vec).construct_buffer_or_rebind(); Span expected_span{const_int_vec.data(), const_int_vec.size()}; using ExpectedValueType = int; - testing::test_const_buffer( + testing::test_const_referencing_buffer( gen_via_const_int_vec, ParameterType::values_on_rank_0, BufferType::in_buffer, @@ -1540,7 +1618,7 @@ TEST(ParameterFactoriesTest, values_on_rank_0_basics_moved_vector) { std::vector const expected = const_int_vec; auto gen_via_moved_vec = values_on_rank_0(std::move(const_int_vec)).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_moved_vec, ParameterType::values_on_rank_0, BufferType::in_buffer, @@ -1556,7 +1634,7 @@ TEST(ParameterFactoriesTest, values_on_rank_0_basics_vector_from_function) { std::vector const expected = make_vector(); auto gen_via_vec_from_function = values_on_rank_0(make_vector()).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_vec_from_function, ParameterType::values_on_rank_0, BufferType::in_buffer, @@ -1568,7 +1646,7 @@ TEST(ParameterFactoriesTest, values_on_rank_0_basics_vector_from_initializer_lis std::vector expected = {1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}; auto gen_via_vec_from_function = values_on_rank_0({1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1}).construct_buffer_or_rebind(); using ExpectedValueType = int; - testing::test_owning_buffer( + testing::test_const_owning_buffer( gen_via_vec_from_function, ParameterType::values_on_rank_0, BufferType::in_buffer, diff --git a/tests/parameter_objects_test.cpp b/tests/parameter_objects_test.cpp index f87976a7e..bf73f1d99 100644 --- a/tests/parameter_objects_test.cpp +++ b/tests/parameter_objects_test.cpp @@ -92,3 +92,24 @@ TEST(ParameterObjectsTest, DataBufferBuilder_with_noncopyable_type) { EXPECT_TRUE(is_non_copyable_own_container>); } } + +TEST(ParameterObjectsTest, DataBufferBuilder_with_const_owning_noncopyable_type) { + { + testing::NonCopyableOwnContainer container{1, 2, 3, 4}; + auto b = make_data_buffer_builder< + kamping::internal::ParameterType::recv_buf, + kamping::internal::BufferModifiability::constant, + kamping::internal::BufferType::out_buffer, + no_resize>(std::move(container)); + container.resize(1); + container[0] = 42; + EXPECT_EQ(b.size(), 4); + auto buffer = b.construct_buffer_or_rebind(); + constexpr bool is_data_ptr_constant = std::is_const_v>; + constexpr bool is_underlying_constant = std::is_const_v>; + EXPECT_TRUE(is_data_ptr_constant); + EXPECT_TRUE(is_underlying_constant); + EXPECT_THAT(buffer.underlying(), ElementsAre(1, 2, 3, 4)); + EXPECT_THAT(buffer.extract(), ElementsAre(1, 2, 3, 4)); + } +}