[libcxx] Throw correct exception from std::vector::reserve

[miyuki]

Summary

According to the standard [vector.capacity]/5, std::vector<T>::reserve
shall throw an exception of type std::length_error when the requested
capacity exceeds max_size().

This behavior is not implemented correctly: the function 'reserve'
simply propagates the exception from allocator<T>::allocate. Before
D110846 that exception used to be of type std::length_error (which is
correct for vector<T>::reserve, but incorrect for
allocator<T>::allocate).

This patch fixes the issue and adds regression tests.

[ldionne]

libcxx/include/vector

1592–1594

Do we need to fix this in vector<bool> too?

libcxx/test/libcxx/containers/sequences/vector.bool/reserve.pass.cpp

1 ↗

(On Diff #380661)

I assume you thought about it and there's no way to test this without relying on libc++ internal details? It would be great if there was a way to avoid that.

[• Quuxplusone]

libcxx/test/libcxx/containers/sequences/vector.bool/reserve.pass.cpp

33 ↗

(On Diff #380661)

I don't even think this use of an internal detail is correct. This should be std::vector<bool, limited_allocator<bool, max_cap_words> > — the allocator's value_type should be the value_type of the container. vector<bool> will rebind the allocator internally as an implementation detail, but the user doesn't see that and in fact isn't allowed to pass in a wrongly-typed allocator AFAIK — I think this code is "library UB" right now.

I think the answer is to pass in a limited_allocator<bool, 10>, and then try to v.reserve(10 * 64 * 100), which "ought to be big enough for anybody," even some hypothetical system where vector<bool>::__storage_type is 128-bit or 256-bit or 6399-bit. (Premise: I don't think we need to test that the happy path on line 43 doesn't throw. We already test that in plenty of places, right? But actually, even if we wanted to test that, v.reserve(10) should still hit the happy path.)

[miyuki]

1. Addressed reviewers' comments.
2. Added a prerequisite patch.

[• Quuxplusone]

LGTM now! (Of course now D112150 blocks this one: but hey, good, we fixed another bug too. :))

libcxx/test/std/containers/sequences/vector.bool/reserve.pass.cpp

67

reasonable

libcxx/include/vector

vector<_Tp, _Allocator>::at(size_type __n) const
     return this->__begin_[__n];
 }
 
 template <class _Tp, class _Allocator>
 void
 vector<_Tp, _Allocator>::reserve(size_type __n)
 {
     if (__n > capacity())
     {
+        if (__n > max_size())
+            this->__throw_length_error();
         allocator_type& __a = this->__alloc();
         __split_buffer<value_type, allocator_type&> __v(__n, size(), __a);
         __swap_out_circular_buffer(__v);
     }
 }
 
 template <class _Tp, class _Allocator>
 void

 }
 
 template <class _Allocator>
 void
 vector<bool, _Allocator>::reserve(size_type __n)
 {
     if (__n > capacity())
     {
+        if (__n > max_size())
+            this->__throw_length_error();
         vector __v(this->get_allocator());
         __v.__vallocate(__n);
         __v.__construct_at_end(this->begin(), this->end());
         swap(__v);
         __invalidate_all_iterators();
     }
 }
 

libcxx/test/std/containers/sequences/vector.bool/reserve.pass.cpp

 
 // void reserve(size_type n);
 
 #include <vector>
 #include <cassert>
 
 #include "test_macros.h"
 #include "min_allocator.h"
+#include "test_allocator.h"
 
 int main(int, char**)
 {
     {
         std::vector<bool> v;
         v.reserve(10);
         assert(v.capacity() >= 10);
     }
#if TEST_STD_VER >= 11
         v.reserve(50);
         assert(v.size() == 100);
         assert(v.capacity() >= 100);
         v.reserve(150);
         assert(v.size() == 100);
         assert(v.capacity() >= 150);
     }
 #endif
+#ifndef TEST_HAS_NO_EXCEPTIONS
+    {
+        std::vector<bool, limited_allocator<bool, 10> > v;
+        v.reserve(5);
+        try {
+            // A typical implementation would allocate chunks of bits.
+            // In libc++ the chunk has the same size as the machine word. It is
+            // reasonable to assume that in practice no implementation would use
+            // 64 kB or larger chunks.
+            v.reserve(10 * 65536);
+            assert(false);
+        } catch (const std::length_error&) {
+            // no-op
+        }
+        assert(v.capacity() >= 5);
+    }
+#endif
 
   return 0;
 }

libcxx/test/std/containers/sequences/vector/vector.capacity/reserve.pass.cpp

#if TEST_STD_VER >= 11
         assert(v.size() == 100);
         assert(v.capacity() == 100);
         v.reserve(150);
         assert(v.size() == 100);
         assert(v.capacity() == 150);
         assert(is_contiguous_container_asan_correct(v));
     }
 #endif
+#ifndef TEST_HAS_NO_EXCEPTIONS
+    {
+        std::vector<int, limited_allocator<int, 100> > v;
+        v.reserve(50);
+        assert(v.capacity() == 50);
+        assert(is_contiguous_container_asan_correct(v));
+        try {
+            v.reserve(101);
+            assert(false);
+        } catch (const std::length_error&) {
+            // no-op
+        }
+        assert(v.capacity() == 50);
+        assert(is_contiguous_container_asan_correct(v));
+    }
+#endif
 
   return 0;
 }