Fix issues with bool VariableLengthArray (issues 74 and 75) (#76)

* Fix issues with bool VariableLengthArray (issues 74 and 75)

Fix iterator range constructor signatures to be consistent
with type T constructors.
Consistently treat size and max size in terms of number of bits.

* Updates from feedback, fix exceptions flag in verify.py

build-tools/bin/verify.py

@@ -692,6 +692,8 @@ def _cmake_configure(args: argparse.Namespace, cmake_args: typing.List[str]) ->
         cmake_configure_args.append("-DLIBCXX_ENABLE_ASSERTIONS:BOOL=ON")
 
     # --[EXCEPTIONS]---------------------------------------
+    if args.exceptions:
+        cmake_configure_args.append("-DCETLVAST_DISABLE_CPP_EXCEPTIONS:BOOL=OFF")
     if args.no_exceptions:
         cmake_configure_args.append("-DCETLVAST_DISABLE_CPP_EXCEPTIONS:BOOL=ON")
 

cetlvast/suites/unittest/test_variable_length_array_bool.cpp

@@ -55,17 +55,31 @@ class VLABoolTests : public ::testing::Test
     }
 };
 
+template <typename T>
+class VLABoolTestsVLAOnly : public ::testing::Test
+{
+protected:
+    void TearDown() override
+    {
+        T::reset();
+    }
+};
+
 // clang-format off
 namespace cetlvast
 {
-using MyTypes = ::testing::Types<
+using VLAAndVectorTypes = ::testing::Types<
       TypeParamDef<cetlvast::PolymorphicAllocatorNewDeleteFactory, cetl::VariableLengthArray, unsigned char>
     , TypeParamDef<cetlvast::DefaultAllocatorFactory, std::vector, unsigned char>
 >;
+using VLATypes = ::testing::Types<
+      TypeParamDef<cetlvast::PolymorphicAllocatorNewDeleteFactory, cetl::VariableLengthArray, unsigned char>
+>;
 }  // namespace cetlvast
 // clang-format on
 
-TYPED_TEST_SUITE(VLABoolTests, cetlvast::MyTypes, );
+TYPED_TEST_SUITE(VLABoolTests, cetlvast::VLAAndVectorTypes, );
+TYPED_TEST_SUITE(VLABoolTestsVLAOnly, cetlvast::VLATypes, );
 
 // +---------------------------------------------------------------------------+
 // | TEST CASES :: Copy Construction
@@ -364,3 +378,43 @@ TYPED_TEST(VLABoolTests, TestAssignCountAndValue)
         ASSERT_TRUE(*i);
     }
 }
+
+TYPED_TEST(VLABoolTestsVLAOnly, ConstructFromIteratorRange)
+{
+    // Provide it too much stuff
+    std::vector<bool> data{false, true, false};
+
+#if defined(__cpp_exceptions)
+    EXPECT_THROW((void)TypeParam::make_bool_container(data.begin(), data.end(), 2U), std::length_error);
+#elif (__cplusplus == CETL_CPP_STANDARD_14)
+    auto subject = TypeParam::make_bool_container(data.begin(), data.end(), 2U);
+    // Should only add to max
+    EXPECT_EQ(subject.size(), 2);
+    EXPECT_EQ(subject[0], false);
+    EXPECT_EQ(subject[1], true);
+#else
+    GTEST_SKIP() << "C++17 pmr does not support defined out of memory behaviour without exceptions.";
+#endif
+}
+
+TYPED_TEST(VLABoolTestsVLAOnly, ExceedMaxSizeMaxFails)
+{
+    std::size_t max = 3U;
+    auto subject = TypeParam::make_bool_container(max);
+    for (std::size_t i = 0; i < max; i++) {
+        subject.push_back(true);
+    }
+#if defined(__cpp_exceptions)
+    EXPECT_THROW((void)subject.push_back(true), std::length_error);
+#elif (__cplusplus == CETL_CPP_STANDARD_14)
+    // Try to add one too many
+    subject.push_back(true);
+    // Shouldn't have been added
+    EXPECT_EQ(subject.size(), 3);
+    EXPECT_EQ(subject[0], true);
+    EXPECT_EQ(subject[1], true);
+    EXPECT_EQ(subject[2], true);
+#else
+    GTEST_SKIP() << "C++17 pmr does not support defined out of memory behaviour without exceptions.";
+#endif
+}

cetlvast/suites/unittest/test_variable_length_array_compat.cpp

@@ -46,7 +46,7 @@ TYPED_TEST(VLATestsCompatPrimitiveTypes, TestMoveToVector)
     cetl::VariableLengthArray<TypeParam, cetl::pf17::pmr::polymorphic_allocator<TypeParam>>
         subject{10u, cetl::pf17::pmr::polymorphic_allocator<TypeParam>(cetl::pf17::pmr::new_delete_resource())};
     subject.reserve(subject.max_size());
-    ASSERT_EQ(subject.capacity(), subject.max_size());
+    ASSERT_GE(subject.capacity(), subject.max_size());
     for (std::size_t i = 0; i < subject.max_size(); ++i)
     {
         subject.push_back(static_cast<TypeParam>(i % 2));

include/cetl/variable_length_array.hpp

@@ -1811,7 +1811,7 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
     /// Required constructor for std::uses_allocator protocol.
     /// @param alloc Allocator to use for all allocations.
     explicit constexpr VariableLengthArray(const allocator_type& alloc) noexcept
-        : VariableLengthArray(std::numeric_limits<size_type>::max(), alloc)
+        : VariableLengthArray(numeric_limits_max_bits(), alloc)
     {
     }
 
@@ -1825,45 +1825,41 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
         : Base(alloc, nullptr, 0, 0, max_size_max)
         , last_byte_bit_fill_{0}
     {
-        Base::reserve(bits2bytes(l.size()), max_size());
+        reserve_bits(l.size(), max_size());
         for (auto list_item : l)
         {
-            emplace_back_impl(list_item);
+            emplace_back(list_item);
         }
     }
 
     /// Initializer syntax constructor.
     /// @param l Initializer list of elements to copy into the array.
     /// @param alloc Allocator to use for all allocations.
     VariableLengthArray(std::initializer_list<bool> l, const allocator_type& alloc)
-        : VariableLengthArray(l, std::numeric_limits<size_type>::max(), alloc)
+        : VariableLengthArray(l, numeric_limits_max_bits(), alloc)
     {
     }
 
     /// Range constructor with maximum max size.
     /// @tparam InputIt The type of the range's iterators.
     /// @param first    The beginning of the range.
     /// @param last     The end of the range.
-    /// @param length   The number of elements to copy from the range.
     /// @param max_size_max Clamping value for the maximum size of this array. That is,
     ///                     cetl::VariableLengthArray::max_size() will return `std::min(max_size_max,
     ///                     std::allocator_traits<allocator_type>::max_size(alloc))`
     /// @param alloc    Allocator to use for all allocations.
     template <class InputIt>
-    VariableLengthArray(InputIt               first,
-                        InputIt               last,
-                        const size_type       length,
-                        size_type             max_size_max,
-                        const allocator_type& alloc)
+    VariableLengthArray(InputIt first, InputIt last, size_type max_size_max, const allocator_type& alloc)
         : Base(alloc, nullptr, 0, 0, max_size_max)
         , last_byte_bit_fill_{0}
     {
         if (last >= first)
         {
-            Base::reserve(bits2bytes(length), max_size());
-            for (size_t inserted = 0; first != last && inserted < length; ++first)
+            const size_type length = static_cast<std::size_t>(last - first);
+            reserve_bits(length, max_size());
+            for (; first != last; ++first)
             {
-                emplace_back_impl(*first);
+                emplace_back(*first);
             }
         }
     }
@@ -1872,11 +1868,10 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
     /// @tparam InputIt The type of the range's iterators.
     /// @param first    The beginning of the range.
     /// @param last     The end of the range.
-    /// @param length   The number of elements to copy from the range.
     /// @param alloc    Allocator to use for all allocations.
     template <class InputIt>
-    VariableLengthArray(InputIt first, InputIt last, const size_type length, const allocator_type& alloc)
-        : VariableLengthArray(first, last, length, std::numeric_limits<size_type>::max(), alloc)
+    VariableLengthArray(InputIt first, InputIt last, const allocator_type& alloc)
+        : VariableLengthArray(first, last, numeric_limits_max_bits(), alloc)
     {
     }
 
@@ -1887,7 +1882,7 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
         : Base(rhs, alloc)
         , last_byte_bit_fill_{rhs.last_byte_bit_fill_}
     {
-        Base::reserve(rhs.size(), rhs.max_size());
+        reserve_bits(rhs.size(), rhs.max_size());
         size_ = Base::fast_copy_construct(data_, capacity_, rhs.data_, rhs.size_, alloc_);
     }
 
@@ -2126,14 +2121,18 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
     /// is out of memory this method will still return the maximum number of elements that could
     /// be stored if the allocator had enough memory, however, it will always return the maximum
     /// size passed into the constructor if that value is less than the allocator's max_size.
-    ///
     /// @return The maximum number of elements that could be stored in this container.
     constexpr size_type max_size() const noexcept
     {
-        const size_type max_diff = std::numeric_limits<ptrdiff_t>::max() / sizeof(bool);
-        const size_type max_size_bytes =
-            std::min(max_size_max_, std::min(max_diff, std::allocator_traits<allocator_type>::max_size(alloc_)));
-        return max_size_bytes * 8;
+        // We can't simply take the maximum possible bytes and convert that to bits because
+        // it could overflow size_type.  Instead we calculate the lesser of the maximum bytes that
+        // could be allocated and the maximum bytes that can be represented and convert that into
+        // bits.
+        const size_type max_diff_bytes = std::numeric_limits<ptrdiff_t>::max() / sizeof(Storage);
+        const size_type max_alloc_bytes =
+            std::min(max_diff_bytes, std::allocator_traits<allocator_type>::max_size(alloc_));
+        const size_type max_bytes = std::min(max_alloc_bytes, numeric_limits_max_bits() / 8U);
+        return std::min(max_size_max_bits(), max_bytes * 8U);
     }
 
     /// Reduce the amount of memory held by this object to the minimum required
@@ -2185,7 +2184,7 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
     ///
     void reserve(const size_type desired_capacity)
     {
-        Base::reserve(bits2bytes(desired_capacity), max_size());
+        reserve_bits(desired_capacity, max_size());
     }
 
     // +----------------------------------------------------------------------+
@@ -2310,13 +2309,25 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
     /// @throw std::bad_alloc if the container cannot obtain enough memory to size up to `count`.
     constexpr void resize(size_type count, const bool value)
     {
+        size_type clamped_count = count;
+        if (clamped_count > max_size())
+        {
+#if defined(__cpp_exceptions)
+            throw std::length_error("Requested count exceeds maximum size.");
+#else
+            // deviation from the standard: instead of undefined behaviour we clamp the count to the maximum size
+            // when exceptions are disabled.
+            clamped_count = max_size();
+#endif
+        }
+
         const std::size_t current_count = size_bits();
-        if (count != current_count)
+        if (clamped_count != current_count)
         {
-            const std::size_t byte_sized = bits2bytes(count);
+            const std::size_t byte_sized = bits2bytes(clamped_count);
             if (byte_sized > capacity_)
             {
-                Base::reserve(byte_sized, max_size());
+                reserve_bits(clamped_count, max_size());
             }
             if (byte_sized == 0)
             {
@@ -2325,7 +2336,7 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
             }
             else
             {
-                Storage bit_sized = (count - 1) % 8U;
+                Storage bit_sized = (clamped_count - 1) % 8U;
                 if (byte_sized > size_)
                 {
                     // Go ahead and just set all bits in the last bytes since we use masks
@@ -2376,7 +2387,8 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
         {
             // we have at least one byte of capacity (first allocation)
             // and we have room in the last byte or we have room for another byte
-            return true;
+            // but we haven't reached the max
+            return size_bits() < max_size();
         }
 
         return Base::grow(max_size());
@@ -2404,9 +2416,25 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
         return true;
     }
 
+    constexpr void reserve_bits(const size_type desired_capacity, const size_type max_size)
+    {
+        size_type clamped_capacity = desired_capacity;
+        if (clamped_capacity > max_size)
+        {
+#if defined(__cpp_exceptions)
+            throw std::length_error("Requested capacity exceeds maximum size.");
+#else
+            // deviation from the standard: instead of undefined behaviour we clamp the capacity to the maximum size
+            // when exceptions are disabled.
+            clamped_capacity = max_size;
+#endif
+        }
+        Base::reserve(bits2bytes(clamped_capacity), bits2bytes(max_size));
+    }
+
     constexpr static size_type round8(const size_type value) noexcept
     {
-        return (value + 7U) & ~7U;
+        return (value + 7U) & ~static_cast<size_type>(7U);  // Careful, make sure the mask is long enough
     }
     constexpr static size_type bits2bytes(const size_type value) noexcept
     {
@@ -2422,11 +2450,32 @@ class VariableLengthArray<bool, Allocator> : protected VariableLengthArrayBase<u
         return (size_ == 0) ? 0 : ((size_ - 1) * 8U) + (last_byte_bit_fill_ + 1U);
     }
 
+    /// Note that the underlying size_ and capacity_ of VariableLengthArrayBase are in terms
+    /// of bytes and then converted to bits in size_bits() and capacity_bits().  However
+    /// bool VLA (unlike type T VLA) treats max_size_max_ in terms of bits and thus no conversion
+    /// is needed.
+    constexpr size_type max_size_max_bits() const noexcept
+    {
+        return max_size_max_;
+    }
+
     constexpr size_type capacity_bits() const noexcept
     {
         return capacity_ * 8U;
     }
 
+    constexpr static size_type numeric_limits_max_bits()
+    {
+        // Make sure it ends on a byte boundary because of the various maths in other places that
+        // convert between bits and bytes
+        const size_type max_bits = std::numeric_limits<size_type>::max() - (std::numeric_limits<size_type>::max() % 8U);
+
+        // numeric_limits_max_bits() passes through round8() so we need to make sure it doesn't overflow
+        static_assert(max_bits + 7U > max_bits, "Uh oh, overflow");
+
+        return max_bits;
+    }
+
     /// The number of bits that are valid in the last byte of the array. If size_ == 0 this value
     /// has no meaning, however, it should always be 0 if size_ == 0 such that, when size_ == 0, size_++
     /// will immediately be valid as 1-bit in the first byte. See size_bits() implementation for more