Fix exceptions from allocator-taking VLA move construction

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<TypeParam>()))), "Must be no-throw move constructable.");
 }
 
-// used by MoveAssignmentNoexcept test.
+// Used by the allocator-dependent move exception specification tests.
 template <typename T, typename isaType, typename pocmaType>
 struct FakeAllocator
 {
@@ -56,6 +56,31 @@ struct FakeAllocator
     };
 };
 
+TYPED_TEST(TestVariableLengthArrayCompiles, MoveConstructorWithAllocatorNoexcept)
+{
+    using AlwaysEqualPropagating = FakeAllocator<typename TypeParam::value_type, std::true_type, std::true_type>;
+    using AlwaysEqual            = FakeAllocator<typename TypeParam::value_type, std::true_type, std::false_type>;
+    using Propagating            = FakeAllocator<typename TypeParam::value_type, std::false_type, std::true_type>;
+    using Unequal                = FakeAllocator<typename TypeParam::value_type, std::false_type, std::false_type>;
+
+    using VLA0 = cetl::VariableLengthArray<typename TypeParam::value_type, AlwaysEqualPropagating>;
+    using VLA1 = cetl::VariableLengthArray<typename TypeParam::value_type, AlwaysEqual>;
+    using VLA2 = cetl::VariableLengthArray<typename TypeParam::value_type, Propagating>;
+    using VLA3 = cetl::VariableLengthArray<typename TypeParam::value_type, Unequal>;
+
+    static_assert(std::is_nothrow_constructible<VLA0, VLA0&&, const typename VLA0::allocator_type&>::value,
+                  "Transferring storage must remain noexcept.");
+    static_assert(std::is_nothrow_constructible<VLA1, VLA1&&, const typename VLA1::allocator_type&>::value,
+                  "Transferring storage must remain noexcept.");
+    static_assert(!std::is_nothrow_constructible<VLA2, VLA2&&, const typename VLA2::allocator_type&>::value,
+                  "Propagation on move assignment is irrelevant to construction: a potentially unequal allocator may "
+                  "allocate and throw.");
+    static_assert(!std::is_nothrow_constructible<VLA3, VLA3&&, const typename VLA3::allocator_type&>::value,
+                  "Moving with a potentially unequal allocator may allocate and throw.");
+    static_assert(std::is_nothrow_move_constructible<VLA3>::value,
+                  "Moving without a supplied allocator must remain noexcept.");
+}
+
 #if defined(__GNUG__)
 #    pragma GCC diagnostic push
 #    if __GNUC__ >= 13

cetlvast/suites/unittest/test_variable_length_array_copy_and_move.cpp

@@ -18,6 +18,160 @@
 #include <vector>
 #include <array>
 #include <type_traits>
+#include <new>
+
+#if defined(__cpp_exceptions)
+
+namespace
+{
+struct MoveConstructorAllocatorState
+{
+    bool        fail_allocation         = false;
+    std::size_t allocation_attempts     = 0;
+    std::size_t outstanding_allocations = 0;
+};
+
+// Never always-equal. Pocma selects propagate_on_container_move_assignment, which must have no bearing on
+// allocator-extended move construction.
+template <typename T, typename Pocma>
+struct MoveConstructorAllocator
+{
+    using value_type                             = T;
+    using is_always_equal                        = std::false_type;
+    using propagate_on_container_move_assignment = Pocma;
+
+    explicit MoveConstructorAllocator(MoveConstructorAllocatorState& state) noexcept
+        : state_(&state)
+    {
+    }
+
+    template <typename U>
+    MoveConstructorAllocator(const MoveConstructorAllocator<U, Pocma>& 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<T>{}.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<T>{}.deallocate(pointer, count);
+        }
+    }
+
+    template <typename U>
+    bool operator==(const MoveConstructorAllocator<U, Pocma>& rhs) const noexcept
+    {
+        return state_ == rhs.state_;
+    }
+
+    template <typename U>
+    bool operator!=(const MoveConstructorAllocator<U, Pocma>& rhs) const noexcept
+    {
+        return !(*this == rhs);
+    }
+
+    MoveConstructorAllocatorState* state_;
+};
+
+template <typename T, typename Pocma>
+struct MoveConstructorParams
+{
+    using value_type = T;
+    using pocma      = Pocma;
+};
+
+template <typename Params>
+class VLAMoveConstructorExceptionTests : public ::testing::Test
+{
+protected:
+    using T         = typename Params::value_type;
+    using Subject   = cetl::VariableLengthArray<T, MoveConstructorAllocator<T, typename Params::pocma>>;
+    using Allocator = typename Subject::allocator_type;
+};
+
+using MoveConstructorValueTypes = ::testing::Types<MoveConstructorParams<int, std::false_type>,
+                                                   MoveConstructorParams<bool, std::false_type>,
+                                                   MoveConstructorParams<int, std::true_type>,
+                                                   MoveConstructorParams<bool, std::true_type>>;
+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

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 <typename UAlloc>
     constexpr VariableLengthArrayBase(
         VariableLengthArrayBase&& rhs,
         const UAlloc&             rhs_alloc,
-        typename std::enable_if_t<is_pocma_or_is_always_equal<UAlloc>::value>* = nullptr) noexcept
+        typename std::enable_if_t<std::allocator_traits<UAlloc>::is_always_equal::value>* = nullptr) noexcept
         : alloc_(std::allocator_traits<UAlloc>::select_on_container_copy_construction(rhs_alloc))
         , data_(std::move(rhs.data_))
         , capacity_(rhs.capacity_)
@@ -814,11 +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 <typename UAlloc>
     constexpr VariableLengthArrayBase(
         VariableLengthArrayBase&& rhs,
         const UAlloc&             rhs_alloc,
-        typename std::enable_if_t<!is_pocma_or_is_always_equal<UAlloc>::value>* = nullptr) noexcept
+        typename std::enable_if_t<!std::allocator_traits<UAlloc>::is_always_equal::value>* = nullptr)
         : alloc_(std::allocator_traits<UAlloc>::select_on_container_copy_construction(rhs_alloc))
         , data_{nullptr}
         , capacity_(0)
@@ -1042,7 +1047,8 @@ class VariableLengthArray : protected VariableLengthArrayBase<T, Allocator>
         return *this;
     }
 
-    VariableLengthArray(VariableLengthArray&& rhs, const allocator_type& alloc) noexcept
+    VariableLengthArray(VariableLengthArray&& rhs, const allocator_type& alloc) noexcept(
+        std::allocator_traits<allocator_type>::is_always_equal::value)
         : Base(std::move(rhs), alloc)
     {
     }
@@ -1910,7 +1916,8 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
         rhs.last_byte_bit_fill_ = 0;
     }
 
-    VariableLengthArray(VariableLengthArray&& rhs, const allocator_type& alloc) noexcept
+    VariableLengthArray(VariableLengthArray&& rhs, const allocator_type& alloc) noexcept(
+        std::allocator_traits<allocator_type>::is_always_equal::value)
         : Base(std::move(rhs), alloc)
         , last_byte_bit_fill_{rhs.last_byte_bit_fill_}
     {