From 5791ae1f711c8a38d2065b35ddcad3daef8b5869 Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Tue, 27 Aug 2024 15:43:24 -0400 Subject: [PATCH] [aa_inplace_vector] Fix some bugs in (unusual) allocator propagation --- include/sg14/aa_inplace_vector.h | 10 +- test/CMakeLists.txt | 3 +- test/aa_inplace_vector_smallsize_test.cpp | 1 + test/aa_inplace_vector_test.cpp | 108 +++++++++++++++++++++- 4 files changed, 113 insertions(+), 9 deletions(-) diff --git a/include/sg14/aa_inplace_vector.h b/include/sg14/aa_inplace_vector.h index 03940d8..d6a4920 100644 --- a/include/sg14/aa_inplace_vector.h +++ b/include/sg14/aa_inplace_vector.h @@ -385,7 +385,7 @@ struct SG14_INPLACE_VECTOR_TRIVIALLY_RELOCATABLE_IF((sg14::aaipv::be_trivially_r std::is_trivially_destructible_v && sg14::aaipv::has_trivial_construct::value && sg14::aaipv::has_trivial_destroy::value) || (N == 0)) && - (std::allocator_traits::propagate_on_container_copy_assignment::value || std::allocator_traits::is_always_equal::value); + std::allocator_traits::is_always_equal::value; static constexpr bool MoveAssignIsDefaultable = ((std::is_trivially_move_constructible_v && @@ -393,7 +393,7 @@ struct SG14_INPLACE_VECTOR_TRIVIALLY_RELOCATABLE_IF((sg14::aaipv::be_trivially_r std::is_trivially_destructible_v && sg14::aaipv::has_trivial_construct::value && sg14::aaipv::has_trivial_destroy::value) || (N == 0)) && - (std::allocator_traits::propagate_on_container_move_assignment::value || std::allocator_traits::is_always_equal::value); + std::allocator_traits::is_always_equal::value; static constexpr bool DtorIsDefaultable = ((std::is_trivially_destructible_v && @@ -546,11 +546,11 @@ struct SG14_INPLACE_VECTOR_TRIVIALLY_RELOCATABLE_IF((sg14::aaipv::be_trivially_r #if defined(__cpp_lib_trivially_relocatable) size_t n = a.size_; a.set_size_(b.size_); - sg14::aaipv::uninitialized_relocate_a(get_allocator_(), a.data_ + b.size_, a.data_ + n, b.data_ + b.size_); + sg14::aaipv::uninitialized_relocate_a(a.get_allocator_(), a.data_ + b.size_, a.data_ + n, b.data_ + b.size_); b.set_size_(n); #else - sg14::aaipv::uninitialized_move_a(get_allocator_(), a.data_ + b.size_, a.data_ + a.size_, b.data_ + b.size_); - sg14::aaipv::destroy_a(get_allocator_(), a.data_ + b.size_, a.data_ + a.size_); + sg14::aaipv::uninitialized_move_a(a.get_allocator_(), a.data_ + b.size_, a.data_ + a.size_, b.data_ + b.size_); + sg14::aaipv::destroy_a(a.get_allocator_(), a.data_ + b.size_, a.data_ + a.size_); a.swap_sizes_(b); #endif } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 38fb7f2..d28bd78 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -3,10 +3,10 @@ find_package(GTest REQUIRED) include(GoogleTest) add_executable(utest - aa_inplace_vector_test.cpp aa_inplace_vector_pmr_test.cpp aa_inplace_vector_smallsize_test.cpp aa_inplace_vector_stdallocator_test.cpp + aa_inplace_vector_test.cpp flat_map_test.cpp flat_set_test.cpp hive_test.cpp @@ -51,6 +51,7 @@ elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") set_property(SOURCE aa_inplace_vector_pmr_test.cpp APPEND PROPERTY COMPILE_OPTIONS "-wd4127") set_property(SOURCE aa_inplace_vector_smallsize_test.cpp APPEND PROPERTY COMPILE_OPTIONS "-wd4127") set_property(SOURCE aa_inplace_vector_stdallocator_test.cpp APPEND PROPERTY COMPILE_OPTIONS "-wd4127") + set_property(SOURCE aa_inplace_vector_test.cpp APPEND PROPERTY COMPILE_OPTIONS "-wd4127") set_property(SOURCE hive_test.cpp APPEND PROPERTY COMPILE_OPTIONS "-wd4127") set_property(SOURCE inplace_vector_test.cpp APPEND PROPERTY COMPILE_OPTIONS "-wd4127") # MSVC gives bogus "possible loss of data" warnings diff --git a/test/aa_inplace_vector_smallsize_test.cpp b/test/aa_inplace_vector_smallsize_test.cpp index 5c10e0c..1e2b181 100644 --- a/test/aa_inplace_vector_smallsize_test.cpp +++ b/test/aa_inplace_vector_smallsize_test.cpp @@ -14,6 +14,7 @@ struct SmallSizeAllocator { // not-an-allocator from the Standard's point of view. SG14 is happy // with it, though. }; +static_assert(std::allocator_traits>::is_always_equal::value); namespace smallsize { template using inplace_vector = sg14::inplace_vector>; diff --git a/test/aa_inplace_vector_test.cpp b/test/aa_inplace_vector_test.cpp index 099f673..8f59090 100644 --- a/test/aa_inplace_vector_test.cpp +++ b/test/aa_inplace_vector_test.cpp @@ -3,9 +3,16 @@ #include #include #include -#include #include +struct AssertFail : std::runtime_error { + using std::runtime_error::runtime_error; +}; + +#define SG14_INPLACE_VECTOR_ASSERT_PRECONDITION(x, msg) if (!(x)) throw AssertFail(msg); + +#include + template struct CountingAlloc { using value_type = T; @@ -50,6 +57,27 @@ struct NonNoexceptAlloc { int i_; }; +template +struct PropagatingAllocator { + using value_type = T; + using propagate_on_container_copy_assignment = std::true_type; + using propagate_on_container_move_assignment = std::true_type; + using propagate_on_container_swap = std::true_type; + explicit PropagatingAllocator(int i) : i_(i) {} + int i_ = 0; + friend bool operator==(const PropagatingAllocator&, const PropagatingAllocator&) = default; + + template + void construct(U *p, Args&&... args) { + ::new (p) U(std::forward(args)...); + } + + template + void destroy(U *p) { + p->~U(); + } +}; + TEST(aa_inplace_vector, AllocExtendedCopyCtor) { using A = CountingAlloc; @@ -86,8 +114,8 @@ TEST(aa_inplace_vector, IgnoreNoexceptnessOfAllocator) using T = sg14::inplace_vector>; static_assert(std::is_nothrow_copy_constructible_v); static_assert(std::is_nothrow_move_constructible_v); - static_assert(std::is_nothrow_copy_assignable_v); - static_assert(std::is_nothrow_move_assignable_v); + static_assert(!std::is_nothrow_copy_assignable_v); // because Lakos rule + static_assert(!std::is_nothrow_move_assignable_v); // because Lakos rule static_assert(std::is_nothrow_destructible_v); } { @@ -100,4 +128,78 @@ TEST(aa_inplace_vector, IgnoreNoexceptnessOfAllocator) } } +TEST(aa_inplace_vector, SwapAllocators) +{ + using A = PropagatingAllocator; + static_assert(std::allocator_traits::propagate_on_container_swap::value); + { + using T = sg14::inplace_vector; + T a = T(A(1)); + T b = T(A(1)); + a.swap(b); + swap(a, b); + T c = T(A(2)); + try { a.swap(c); EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "swap tried to swap unequal allocators; this is UB"); } + try { swap(a, c); EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "swap tried to swap unequal allocators; this is UB"); } + } + { + using T = sg14::inplace_vector; + T a = T(A(1)); + T b = T(A(1)); + a.swap(b); + swap(a, b); + T c = T(A(2)); + try { a.swap(c); EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "swap tried to swap unequal allocators; this is UB"); } + try { swap(a, c); EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "swap tried to swap unequal allocators; this is UB"); } + } +} + +TEST(aa_inplace_vector, CopyAssignAllocators) +{ + using A = PropagatingAllocator; + static_assert(std::allocator_traits::propagate_on_container_copy_assignment::value); + { + using T = sg14::inplace_vector; + static_assert(!std::is_trivially_copy_assignable_v); + T a = T(A(1)); + T b = T(A(1)); + a = b; + T c = T(A(2)); + try { a = c; EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "operator= tried to propagate an unequal allocator; this is UB"); } + } + { + using T = sg14::inplace_vector; + static_assert(!std::is_trivially_copy_assignable_v); + T a = T(A(1)); + T b = T(A(1)); + a = b; + T c = T(A(2)); + try { a = c; EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "operator= tried to propagate an unequal allocator; this is UB"); } + } +} + +TEST(aa_inplace_vector, MoveAssignAllocators) +{ + using A = PropagatingAllocator; + static_assert(std::allocator_traits::propagate_on_container_move_assignment::value); + { + using T = sg14::inplace_vector; + static_assert(!std::is_trivially_move_assignable_v); + T a = T(A(1)); + T b = T(A(1)); + a = std::move(b); + T c = T(A(2)); + try { a = std::move(c); EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "operator= tried to propagate an unequal allocator; this is UB"); } + } + { + using T = sg14::inplace_vector; + static_assert(!std::is_trivially_move_assignable_v); + T a = T(A(1)); + T b = T(A(1)); + a = std::move(b); + T c = T(A(2)); + try { a = std::move(c); EXPECT_TRUE(false); } catch (const AssertFail& ex) { EXPECT_STREQ(ex.what(), "operator= tried to propagate an unequal allocator; this is UB"); } + } +} + #endif // __cplusplus >= 202002L