From ff3de7f0726915bfdf1187e677141b45f0b4e940 Mon Sep 17 00:00:00 2001 From: Scott Dixon Date: Tue, 29 Sep 2026 11:44:27 -0700 Subject: [PATCH 1/2] Fix allocator-taking VLA move constructor exception specifications --- .../test_variable_length_array_compiles.cpp | 26 +++- ...st_variable_length_array_copy_and_move.cpp | 141 ++++++++++++++++++ include/cetl/variable_length_array.hpp | 15 +- 3 files changed, 175 insertions(+), 7 deletions(-) diff --git a/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp b/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp index 9f624cde..0bc024ed 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp @@ -41,7 +41,7 @@ TYPED_TEST(TestVariableLengthArrayCompiles, MoveConstructorIsNoThrow) static_assert(noexcept(TypeParam(std::move(std::declval()))), "Must be no-throw move constructable."); } -// used by MoveAssignmentNoexcept test. +// Used by the allocator-dependent move exception specification tests. template struct FakeAllocator { @@ -56,6 +56,30 @@ struct FakeAllocator }; }; +TYPED_TEST(TestVariableLengthArrayCompiles, MoveConstructorWithAllocatorNoexcept) +{ + using AlwaysEqualPropagating = FakeAllocator; + using AlwaysEqual = FakeAllocator; + using Propagating = FakeAllocator; + using Unequal = FakeAllocator; + + using VLA0 = cetl::VariableLengthArray; + using VLA1 = cetl::VariableLengthArray; + using VLA2 = cetl::VariableLengthArray; + using VLA3 = cetl::VariableLengthArray; + + static_assert(std::is_nothrow_constructible::value, + "Transferring storage must remain noexcept."); + static_assert(std::is_nothrow_constructible::value, + "Transferring storage must remain noexcept."); + static_assert(std::is_nothrow_constructible::value, + "Transferring storage must remain noexcept."); + static_assert(!std::is_nothrow_constructible::value, + "Moving with a potentially unequal allocator may allocate and throw."); + static_assert(std::is_nothrow_move_constructible::value, + "Moving without a supplied allocator must remain noexcept."); +} + #if defined(__GNUG__) # pragma GCC diagnostic push # if __GNUC__ >= 13 diff --git a/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp b/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp index 0d83278b..9bd3aef5 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp @@ -18,6 +18,147 @@ #include #include #include +#include + +#if defined(__cpp_exceptions) + +namespace +{ +struct MoveConstructorAllocatorState +{ + bool fail_allocation = false; + std::size_t allocation_attempts = 0; + std::size_t outstanding_allocations = 0; +}; + +template +struct MoveConstructorAllocator +{ + using value_type = T; + using is_always_equal = std::false_type; + using propagate_on_container_move_assignment = std::false_type; + + explicit MoveConstructorAllocator(MoveConstructorAllocatorState& state) noexcept + : state_(&state) + { + } + + template + MoveConstructorAllocator(const MoveConstructorAllocator& rhs) noexcept + : state_(rhs.state_) + { + } + + T* allocate(std::size_t count) + { + ++state_->allocation_attempts; + if (state_->fail_allocation) + { + throw std::bad_alloc(); + } + T* const result = std::allocator{}.allocate(count); + ++state_->outstanding_allocations; + return result; + } + + void deallocate(T* pointer, std::size_t count) noexcept + { + if (pointer != nullptr) + { + EXPECT_GT(state_->outstanding_allocations, 0U); + --state_->outstanding_allocations; + std::allocator{}.deallocate(pointer, count); + } + } + + template + bool operator==(const MoveConstructorAllocator& rhs) const noexcept + { + return state_ == rhs.state_; + } + + template + bool operator!=(const MoveConstructorAllocator& rhs) const noexcept + { + return !(*this == rhs); + } + + MoveConstructorAllocatorState* state_; +}; + +template +class VLAMoveConstructorExceptionTests : public ::testing::Test +{ +protected: + using Subject = cetl::VariableLengthArray>; + using Allocator = typename Subject::allocator_type; +}; + +using MoveConstructorValueTypes = ::testing::Types; +TYPED_TEST_SUITE(VLAMoveConstructorExceptionTests, MoveConstructorValueTypes, ); + +TYPED_TEST(VLAMoveConstructorExceptionTests, UnequalAllocatorAllocationFailure) +{ + using Subject = typename TestFixture::Subject; + using Allocator = typename TestFixture::Allocator; + + MoveConstructorAllocatorState source_state; + MoveConstructorAllocatorState destination_state; + { + Subject source{{1, 0, 1}, Allocator{source_state}}; + const auto original_capacity = source.capacity(); + destination_state.fail_allocation = true; + + EXPECT_THROW((Subject{std::move(source), Allocator{destination_state}}), std::bad_alloc); + ASSERT_EQ(source.size(), 3U); + EXPECT_EQ(source.capacity(), original_capacity); + EXPECT_EQ(source[0], 1); + EXPECT_EQ(source[1], 0); + EXPECT_EQ(source[2], 1); + EXPECT_EQ(source_state.outstanding_allocations, 1U); + EXPECT_EQ(destination_state.allocation_attempts, 1U); + EXPECT_EQ(destination_state.outstanding_allocations, 0U); + + // The failed allocation must leave the source available for a later move. + destination_state.fail_allocation = false; + Subject destination{std::move(source), Allocator{destination_state}}; + ASSERT_EQ(destination.size(), 3U); + EXPECT_EQ(destination[0], 1); + EXPECT_EQ(destination[1], 0); + EXPECT_EQ(destination[2], 1); + EXPECT_TRUE(source.empty()); + EXPECT_EQ(source_state.outstanding_allocations, 0U); + EXPECT_EQ(destination_state.allocation_attempts, 2U); + EXPECT_EQ(destination_state.outstanding_allocations, 1U); + } + EXPECT_EQ(source_state.outstanding_allocations, 0U); + EXPECT_EQ(destination_state.outstanding_allocations, 0U); +} + +TYPED_TEST(VLAMoveConstructorExceptionTests, EqualAllocatorDoesNotAllocate) +{ + using Subject = typename TestFixture::Subject; + using Allocator = typename TestFixture::Allocator; + + MoveConstructorAllocatorState state; + { + Subject source{{1, 0, 1}, Allocator{state}}; + state.fail_allocation = true; + Subject destination{std::move(source), Allocator{state}}; + + EXPECT_TRUE(source.empty()); + ASSERT_EQ(destination.size(), 3U); + EXPECT_EQ(destination[0], 1); + EXPECT_EQ(destination[1], 0); + EXPECT_EQ(destination[2], 1); + EXPECT_EQ(state.allocation_attempts, 1U); + EXPECT_EQ(state.outstanding_allocations, 1U); + } + EXPECT_EQ(state.outstanding_allocations, 0U); +} +} // namespace + +#endif // __cpp_exceptions // +---------------------------------------------------------------------------+ // | TEST VALUE TYPES diff --git a/include/cetl/variable_length_array.hpp b/include/cetl/variable_length_array.hpp index e715a85f..a5f14135 100644 --- a/include/cetl/variable_length_array.hpp +++ b/include/cetl/variable_length_array.hpp @@ -815,10 +815,9 @@ class VariableLengthArrayBase } template - constexpr VariableLengthArrayBase( - VariableLengthArrayBase&& rhs, - const UAlloc& rhs_alloc, - typename std::enable_if_t::value>* = nullptr) noexcept + constexpr VariableLengthArrayBase(VariableLengthArrayBase&& rhs, + const UAlloc& rhs_alloc, + typename std::enable_if_t::value>* = nullptr) : alloc_(std::allocator_traits::select_on_container_copy_construction(rhs_alloc)) , data_{nullptr} , capacity_(0) @@ -1042,7 +1041,9 @@ class VariableLengthArray : protected VariableLengthArrayBase return *this; } - VariableLengthArray(VariableLengthArray&& rhs, const allocator_type& alloc) noexcept + VariableLengthArray(VariableLengthArray&& rhs, const allocator_type& alloc) noexcept( + std::allocator_traits::propagate_on_container_move_assignment::value || + std::allocator_traits::is_always_equal::value) : Base(std::move(rhs), alloc) { } @@ -1910,7 +1911,9 @@ class VariableLengthArray : protected VariableLengthArrayBase::propagate_on_container_move_assignment::value || + std::allocator_traits::is_always_equal::value) : Base(std::move(rhs), alloc) , last_byte_bit_fill_{rhs.last_byte_bit_fill_} { From 25c7deed5c7a0941549ed5822021439642024361 Mon Sep 17 00:00:00 2001 From: Scott Dixon Date: Thu, 1 Oct 2026 14:59:28 -0700 Subject: [PATCH 2/2] Select allocator-extended VLA move by is_always_equal only propagate_on_container_move_assignment applies to assignment and says nothing about whether the allocator passed to a constructor equals the source's. The base move constructor used it to select the overload that adopts the source's storage, so a POCMA allocator that is not always equal took the source's buffer even when the supplied allocator was a different instance. The destination later deallocated that buffer through the wrong allocator, and the public constructors were marked noexcept even though they must allocate in that case. - Select the storage-adopting base constructor only when is_always_equal. Otherwise use the runtime equality check, which allocates when unequal. - Base the public allocator-extended move constructors' noexcept on is_always_equal only, for both VariableLengthArray and the bool specialization. - Expect a POCMA allocator that is not always equal to be potentially throwing in the compile test. - Run the allocation-failure tests with both POCMA settings. With the previous dispatch the POCMA cases don't throw and deallocate through the wrong allocator. Co-Authored-By: Claude Opus 5.5 --- .../test_variable_length_array_compiles.cpp | 5 ++-- ...st_variable_length_array_copy_and_move.cpp | 29 ++++++++++++++----- include/cetl/variable_length_array.hpp | 16 ++++++---- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp b/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp index 0bc024ed..ce4418c1 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_compiles.cpp @@ -72,8 +72,9 @@ TYPED_TEST(TestVariableLengthArrayCompiles, MoveConstructorWithAllocatorNoexcept "Transferring storage must remain noexcept."); static_assert(std::is_nothrow_constructible::value, "Transferring storage must remain noexcept."); - static_assert(std::is_nothrow_constructible::value, - "Transferring storage must remain noexcept."); + static_assert(!std::is_nothrow_constructible::value, + "Propagation on move assignment is irrelevant to construction: a potentially unequal allocator may " + "allocate and throw."); static_assert(!std::is_nothrow_constructible::value, "Moving with a potentially unequal allocator may allocate and throw."); static_assert(std::is_nothrow_move_constructible::value, diff --git a/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp b/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp index 9bd3aef5..9e469905 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp @@ -31,12 +31,14 @@ struct MoveConstructorAllocatorState std::size_t outstanding_allocations = 0; }; -template +// Never always-equal. Pocma selects propagate_on_container_move_assignment, which must have no bearing on +// allocator-extended move construction. +template struct MoveConstructorAllocator { using value_type = T; using is_always_equal = std::false_type; - using propagate_on_container_move_assignment = std::false_type; + using propagate_on_container_move_assignment = Pocma; explicit MoveConstructorAllocator(MoveConstructorAllocatorState& state) noexcept : state_(&state) @@ -44,7 +46,7 @@ struct MoveConstructorAllocator } template - MoveConstructorAllocator(const MoveConstructorAllocator& rhs) noexcept + MoveConstructorAllocator(const MoveConstructorAllocator& rhs) noexcept : state_(rhs.state_) { } @@ -72,13 +74,13 @@ struct MoveConstructorAllocator } template - bool operator==(const MoveConstructorAllocator& rhs) const noexcept + bool operator==(const MoveConstructorAllocator& rhs) const noexcept { return state_ == rhs.state_; } template - bool operator!=(const MoveConstructorAllocator& rhs) const noexcept + bool operator!=(const MoveConstructorAllocator& rhs) const noexcept { return !(*this == rhs); } @@ -86,15 +88,26 @@ struct MoveConstructorAllocator MoveConstructorAllocatorState* state_; }; -template +template +struct MoveConstructorParams +{ + using value_type = T; + using pocma = Pocma; +}; + +template class VLAMoveConstructorExceptionTests : public ::testing::Test { protected: - using Subject = cetl::VariableLengthArray>; + using T = typename Params::value_type; + using Subject = cetl::VariableLengthArray>; using Allocator = typename Subject::allocator_type; }; -using MoveConstructorValueTypes = ::testing::Types; +using MoveConstructorValueTypes = ::testing::Types, + MoveConstructorParams, + MoveConstructorParams, + MoveConstructorParams>; TYPED_TEST_SUITE(VLAMoveConstructorExceptionTests, MoveConstructorValueTypes, ); TYPED_TEST(VLAMoveConstructorExceptionTests, UnequalAllocatorAllocationFailure) diff --git a/include/cetl/variable_length_array.hpp b/include/cetl/variable_length_array.hpp index a5f14135..55d445ca 100644 --- a/include/cetl/variable_length_array.hpp +++ b/include/cetl/variable_length_array.hpp @@ -796,11 +796,13 @@ class VariableLengthArrayBase rhs.data_ = nullptr; } + /// Allocator-extended move construction where the allocators are always equal: the storage is simply adopted. + /// Note that propagate_on_container_move_assignment has no bearing on construction. template constexpr VariableLengthArrayBase( VariableLengthArrayBase&& rhs, const UAlloc& rhs_alloc, - typename std::enable_if_t::value>* = nullptr) noexcept + typename std::enable_if_t::is_always_equal::value>* = nullptr) noexcept : alloc_(std::allocator_traits::select_on_container_copy_construction(rhs_alloc)) , data_(std::move(rhs.data_)) , capacity_(rhs.capacity_) @@ -814,10 +816,14 @@ class VariableLengthArrayBase rhs.data_ = nullptr; } + /// Allocator-extended move construction where the allocators may be unequal. If they are equal at runtime the + /// storage is adopted, otherwise storage is obtained from the given allocator and the elements are moved into + /// it. The latter can throw so this overload is not noexcept. template - constexpr VariableLengthArrayBase(VariableLengthArrayBase&& rhs, - const UAlloc& rhs_alloc, - typename std::enable_if_t::value>* = nullptr) + constexpr VariableLengthArrayBase( + VariableLengthArrayBase&& rhs, + const UAlloc& rhs_alloc, + typename std::enable_if_t::is_always_equal::value>* = nullptr) : alloc_(std::allocator_traits::select_on_container_copy_construction(rhs_alloc)) , data_{nullptr} , capacity_(0) @@ -1042,7 +1048,6 @@ class VariableLengthArray : protected VariableLengthArrayBase } VariableLengthArray(VariableLengthArray&& rhs, const allocator_type& alloc) noexcept( - std::allocator_traits::propagate_on_container_move_assignment::value || std::allocator_traits::is_always_equal::value) : Base(std::move(rhs), alloc) { @@ -1912,7 +1917,6 @@ class VariableLengthArray : protected VariableLengthArrayBase::propagate_on_container_move_assignment::value || std::allocator_traits::is_always_equal::value) : Base(std::move(rhs), alloc) , last_byte_bit_fill_{rhs.last_byte_bit_fill_}