Fix PR37694 - std::vector doesn't correctly move construct allocators.

C++2a[container.requirements.general]p8 states that when move constructing
a container, the allocator is move constructed. Vector previously copy
constructed these allocators. This patch fixes that bug.

Additionally it cleans up some unnecessary allocator conversions
when copy constructing containers. Libc++ uses
__internal_allocator_traits::select_on_copy_construction to select
the correct allocator during copy construction, but it unnecessarily
converted the resulting allocator to the user specified allocator
type and back. After this patch list and forward_list no longer
do that.

Technically we're supposed to be using allocator_traits<allocator_type>::select_on_copy_construction,
but that should seemingly be addressed as a separate patch, if at all.

llvm-svn: 334053

libcxx/include/forward_list

@@ -457,6 +457,10 @@ protected:
     typedef typename allocator_traits<__begin_node_allocator>::pointer
                                                       __begin_node_pointer;
 
+    static_assert((!is_same<allocator_type, __node_allocator>::value),
+                  "internal allocator type must differ from user-specified "
+                  "type; otherwise overload resolution breaks");
+
     __compressed_pair<__begin_node, __node_allocator> __before_begin_;
 
     _LIBCPP_INLINE_VISIBILITY
@@ -481,9 +485,11 @@ protected:
         _NOEXCEPT_(is_nothrow_default_constructible<__node_allocator>::value)
         : __before_begin_(__begin_node()) {}
     _LIBCPP_INLINE_VISIBILITY
-    __forward_list_base(const allocator_type& __a)
+    explicit __forward_list_base(const allocator_type& __a)
         : __before_begin_(__begin_node(), __node_allocator(__a)) {}
-
+    _LIBCPP_INLINE_VISIBILITY
+    explicit __forward_list_base(const __node_allocator& __a)
+        : __before_begin_(__begin_node(), __a) {}
 #ifndef _LIBCPP_CXX03_LANG
 public:
     _LIBCPP_INLINE_VISIBILITY
@@ -954,11 +960,8 @@ forward_list<_Tp, _Alloc>::forward_list(_InputIterator __f, _InputIterator __l,
 
 template <class _Tp, class _Alloc>
 forward_list<_Tp, _Alloc>::forward_list(const forward_list& __x)
-    : base(allocator_type(
-             __node_traits::select_on_container_copy_construction(__x.__alloc())
-                         )
-          )
-{
+    : base(
+          __node_traits::select_on_container_copy_construction(__x.__alloc())) {
   insert_after(cbefore_begin(), __x.begin(), __x.end());
 }
 

libcxx/include/list

@@ -556,6 +556,9 @@ protected:
 
     typedef typename __rebind_alloc_helper<__alloc_traits, __node_base>::type __node_base_allocator;
     typedef typename allocator_traits<__node_base_allocator>::pointer __node_base_pointer;
+    static_assert((!is_same<allocator_type, __node_allocator>::value),
+                  "internal allocator type must differ from user-specified "
+                  "type; otherwise overload resolution breaks");
 
     __node_base __end_;
     __compressed_pair<size_type, __node_allocator> __size_alloc_;
@@ -590,6 +593,11 @@ protected:
         _NOEXCEPT_(is_nothrow_default_constructible<__node_allocator>::value);
     _LIBCPP_INLINE_VISIBILITY
     __list_imp(const allocator_type& __a);
+    _LIBCPP_INLINE_VISIBILITY
+    __list_imp(const __node_allocator& __a);
+#ifndef _LIBCPP_CXX03_LANG
+    __list_imp(__node_allocator&& __a) _NOEXCEPT;
+#endif
     ~__list_imp();
     void clear() _NOEXCEPT;
     _LIBCPP_INLINE_VISIBILITY
@@ -713,8 +721,17 @@ __list_imp<_Tp, _Alloc>::__list_imp(const allocator_type& __a)
 }
 
 template <class _Tp, class _Alloc>
-__list_imp<_Tp, _Alloc>::~__list_imp()
-{
+inline __list_imp<_Tp, _Alloc>::__list_imp(const __node_allocator& __a)
+    : __size_alloc_(0, __a) {}
+
+#ifndef _LIBCPP_CXX03_LANG
+template <class _Tp, class _Alloc>
+inline __list_imp<_Tp, _Alloc>::__list_imp(__node_allocator&& __a) _NOEXCEPT
+    : __size_alloc_(0, std::move(__a)) {}
+#endif
+
+template <class _Tp, class _Alloc>
+__list_imp<_Tp, _Alloc>::~__list_imp() {
   clear();
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__erase_c(this);
@@ -1248,10 +1265,8 @@ list<_Tp, _Alloc>::list(_InpIter __f, _InpIter __l, const allocator_type& __a,
 
 template <class _Tp, class _Alloc>
 list<_Tp, _Alloc>::list(const list& __c)
-    : base(allocator_type(
-           __node_alloc_traits::select_on_container_copy_construction(
-                __c.__node_alloc())))
-{
+    : base(__node_alloc_traits::select_on_container_copy_construction(
+          __c.__node_alloc())) {
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif
@@ -1296,11 +1311,9 @@ list<_Tp, _Alloc>::list(initializer_list<value_type> __il)
 }
 
 template <class _Tp, class _Alloc>
-inline
-list<_Tp, _Alloc>::list(list&& __c)
+inline list<_Tp, _Alloc>::list(list&& __c)
     _NOEXCEPT_(is_nothrow_move_constructible<__node_allocator>::value)
-    : base(allocator_type(_VSTD::move(__c.__node_alloc())))
-{
+    : base(_VSTD::move(__c.__node_alloc())) {
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif

libcxx/include/vector

@@ -355,6 +355,9 @@ protected:
     __vector_base()
         _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value);
     _LIBCPP_INLINE_VISIBILITY __vector_base(const allocator_type& __a);
+#ifndef _LIBCPP_CXX03_LANG
+    _LIBCPP_INLINE_VISIBILITY __vector_base(allocator_type&& __a) _NOEXCEPT;
+#endif
     ~__vector_base();
 
     _LIBCPP_INLINE_VISIBILITY
@@ -438,6 +441,15 @@ __vector_base<_Tp, _Allocator>::__vector_base(const allocator_type& __a)
 {
 }
 
+#ifndef _LIBCPP_CXX03_LANG
+template <class _Tp, class _Allocator>
+inline _LIBCPP_INLINE_VISIBILITY
+__vector_base<_Tp, _Allocator>::__vector_base(allocator_type&& __a) _NOEXCEPT
+    : __begin_(nullptr),
+      __end_(nullptr),
+      __end_cap_(nullptr, std::move(__a)) {}
+#endif
+
 template <class _Tp, class _Allocator>
 __vector_base<_Tp, _Allocator>::~__vector_base()
 {
@@ -2863,8 +2875,7 @@ vector<bool, _Allocator>::operator=(const vector& __v)
 #ifndef _LIBCPP_CXX03_LANG
 
 template <class _Allocator>
-inline _LIBCPP_INLINE_VISIBILITY
-vector<bool, _Allocator>::vector(vector&& __v)
+inline _LIBCPP_INLINE_VISIBILITY vector<bool, _Allocator>::vector(vector&& __v)
 #if _LIBCPP_STD_VER > 14
     _NOEXCEPT
 #else
@@ -2872,8 +2883,7 @@ vector<bool, _Allocator>::vector(vector&& __v)
 #endif
     : __begin_(__v.__begin_),
       __size_(__v.__size_),
-      __cap_alloc_(__v.__cap_alloc_)
-{
+      __cap_alloc_(std::move(__v.__cap_alloc_)) {
     __v.__begin_ = nullptr;
     __v.__size_ = 0;
     __v.__cap() = 0;

libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp

@@ -0,0 +1,97 @@
+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03
+
+// C++2a[container.requirements.general]p8
+//   Move constructors obtain an allocator by move construction from the allocator
+//   belonging to the container being moved. Such move construction of the
+//   allocator shall not exit via an exception.
+
+#include <vector>
+#include <list>
+#include <forward_list>
+#include <set>
+#include <map>
+#include <unordered_map>
+#include <unordered_set>
+
+#include "test_macros.h"
+#include "test_allocator.h"
+
+template <class C>
+void test(int expected_num_allocs = 1) {
+  {
+    test_alloc_base::clear();
+    using AllocT = typename C::allocator_type;
+    C v(AllocT(42, 101));
+
+    assert(test_alloc_base::count == expected_num_allocs);
+
+    const int num_stored_allocs = test_alloc_base::count;
+    {
+      const AllocT& a = v.get_allocator();
+      assert(test_alloc_base::count == 1 + num_stored_allocs);
+      assert(a.get_data() == 42);
+      assert(a.get_id() == 101);
+    }
+    assert(test_alloc_base::count == num_stored_allocs);
+    test_alloc_base::clear_ctor_counters();
+
+    C v2 = std::move(v);
+    assert(test_alloc_base::count == num_stored_allocs * 2);
+    assert(test_alloc_base::copied == 0);
+    assert(test_alloc_base::moved == num_stored_allocs);
+    {
+      const AllocT& a = v.get_allocator();
+      assert(a.get_id() == test_alloc_base::moved_value);
+      assert(a.get_data() == test_alloc_base::moved_value);
+    }
+    {
+      const AllocT& a = v2.get_allocator();
+      assert(a.get_id() == 101);
+      assert(a.get_data() == 42);
+    }
+  }
+}
+
+int main() {
+  { // test sequence containers
+    test<std::vector<int, test_allocator<int> > >();
+    test<std::vector<bool, test_allocator<bool> > >();
+    test<std::list<int, test_allocator<int> > >();
+    test<std::forward_list<int, test_allocator<int> > >();
+  }
+  { // test associative containers
+    test<std::set<int, std::less<int>, test_allocator<int> > >();
+    test<std::multiset<int, std::less<int>, test_allocator<int> > >();
+
+    using KV = std::pair<const int, int>;
+    test<std::map<int, int, std::less<int>, test_allocator<KV> > >();
+    test<std::multimap<int, int, std::less<int>, test_allocator<KV> > >();
+  }
+  { // test unordered containers
+    // libc++ stores two allocators in the unordered containers.
+#ifdef _LIBCPP_VERSION
+    int stored_allocators = 2;
+#else
+    int stored_allocators = 1;
+#endif
+    test<std::unordered_set<int, std::hash<int>, std::equal_to<int>,
+                            test_allocator<int> > >(stored_allocators);
+    test<std::unordered_multiset<int, std::hash<int>, std::equal_to<int>,
+                                 test_allocator<int> > >(stored_allocators);
+
+    using KV = std::pair<const int, int>;
+    test<std::unordered_map<int, int, std::hash<int>, std::equal_to<int>,
+                            test_allocator<KV> > >(stored_allocators);
+    test<std::unordered_multimap<int, int, std::hash<int>, std::equal_to<int>,
+                                 test_allocator<KV> > >(stored_allocators);
+  }
+}

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

@@ -15,6 +15,7 @@
 
 #include <vector>
 #include <cassert>
+#include "test_macros.h"
 #include "test_allocator.h"
 #include "min_allocator.h"
 
@@ -59,4 +60,34 @@ int main()
         assert(l.empty());
         assert(l2.get_allocator() == lo.get_allocator());
     }
+    {
+      test_alloc_base::clear();
+      using Vect = std::vector<bool, test_allocator<bool> >;
+      using AllocT = Vect::allocator_type;
+      Vect v(test_allocator<bool>(42, 101));
+      assert(test_alloc_base::count == 1);
+      {
+        const AllocT& a = v.get_allocator();
+        assert(test_alloc_base::count == 2);
+        assert(a.get_data() == 42);
+        assert(a.get_id() == 101);
+      }
+      assert(test_alloc_base::count == 1);
+      test_alloc_base::clear_ctor_counters();
+
+      Vect v2 = std::move(v);
+      assert(test_alloc_base::count == 2);
+      assert(test_alloc_base::copied == 0);
+      assert(test_alloc_base::moved == 1);
+      {
+        const AllocT& a = v.get_allocator();
+        assert(a.get_id() == test_alloc_base::moved_value);
+        assert(a.get_data() == test_alloc_base::moved_value);
+      }
+      {
+        const AllocT& a = v2.get_allocator();
+        assert(a.get_id() == 101);
+        assert(a.get_data() == 42);
+      }
+    }
 }

libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp

@@ -15,10 +15,13 @@
 
 #include <vector>
 #include <cassert>
+
+#include "test_macros.h"
 #include "MoveOnly.h"
 #include "test_allocator.h"
 #include "min_allocator.h"
 #include "asan_testing.h"
+#include "verbose_assert.h"
 
 int main()
 {
@@ -98,4 +101,34 @@ int main()
         assert(*j == 3);
         assert(is_contiguous_container_asan_correct(c2));
     }
+    {
+      test_alloc_base::clear();
+      using Vect = std::vector<int, test_allocator<int> >;
+      Vect v(test_allocator<int>(42, 101));
+      assert(test_alloc_base::count == 1);
+      assert(test_alloc_base::copied == 1);
+      assert(test_alloc_base::moved == 0);
+      {
+        const test_allocator<int>& a = v.get_allocator();
+        assert(a.get_data() == 42);
+        assert(a.get_id() == 101);
+      }
+      assert(test_alloc_base::count == 1);
+      test_alloc_base::clear_ctor_counters();
+
+      Vect v2 = std::move(v);
+      assert(test_alloc_base::count == 2);
+      assert(test_alloc_base::copied == 0);
+      assert(test_alloc_base::moved == 1);
+      {
+        const test_allocator<int>& a = v.get_allocator();
+        assert(a.get_id() == test_alloc_base::moved_value);
+        assert(a.get_data() == test_alloc_base::moved_value);
+      }
+      {
+        const test_allocator<int>& a = v2.get_allocator();
+        assert(a.get_id() == 101);
+        assert(a.get_data() == 42);
+      }
+    }
 }

libcxx/test/support/test_allocator.h

@@ -36,12 +36,37 @@ public:
     static int throw_after;
     static int count;
     static int alloc_count;
+    static int copied;
+    static int moved;
+    static int converted;
+
+    const static int destructed_value = -1;
+    const static int default_value = 0;
+    const static int moved_value = INT_MAX;
+
+    static void clear() {
+      assert(count == 0 && "clearing leaking allocator data?");
+      count = 0;
+      time_to_throw = 0;
+      alloc_count = 0;
+      throw_after = INT_MAX;
+      clear_ctor_counters();
+    }
+
+    static void clear_ctor_counters() {
+      copied = 0;
+      moved = 0;
+      converted = 0;
+    }
 };
 
 int test_alloc_base::count = 0;
 int test_alloc_base::time_to_throw = 0;
 int test_alloc_base::alloc_count = 0;
 int test_alloc_base::throw_after = INT_MAX;
+int test_alloc_base::copied = 0;
+int test_alloc_base::moved = 0;
+int test_alloc_base::converted = 0;
 
 template <class T>
 class test_allocator
@@ -65,13 +90,35 @@ public:
     test_allocator() TEST_NOEXCEPT : data_(0), id_(0) {++count;}
     explicit test_allocator(int i, int id = 0) TEST_NOEXCEPT : data_(i), id_(id)
       {++count;}
-    test_allocator(const test_allocator& a) TEST_NOEXCEPT
-        : data_(a.data_), id_(a.id_) {++count;}
-    template <class U> test_allocator(const test_allocator<U>& a) TEST_NOEXCEPT
-        : data_(a.data_), id_(a.id_) {++count;}
+    test_allocator(const test_allocator& a) TEST_NOEXCEPT : data_(a.data_),
+                                                            id_(a.id_) {
+      ++count;
+      ++copied;
+      assert(a.data_ != destructed_value && a.id_ != destructed_value &&
+             "copying from destroyed allocator");
+    }
+#if TEST_STD_VER >= 11
+    test_allocator(test_allocator&& a) TEST_NOEXCEPT : data_(a.data_),
+                                                       id_(a.id_) {
+      ++count;
+      ++moved;
+      assert(a.data_ != destructed_value && a.id_ != destructed_value &&
+             "moving from destroyed allocator");
+      a.data_ = moved_value;
+      a.id_ = moved_value;
+    }
+#endif
+    template <class U>
+    test_allocator(const test_allocator<U>& a) TEST_NOEXCEPT : data_(a.data_),
+                                                               id_(a.id_) {
+      ++count;
+      ++converted;
+    }
     ~test_allocator() TEST_NOEXCEPT {
       assert(data_ >= 0); assert(id_ >= 0);
-      --count; data_ = -1; id_ = -1;
+      --count;
+      data_ = destructed_value;
+      id_ = destructed_value;
     }
     pointer address(reference x) const {return &x;}
     const_pointer address(const_reference x) const {return &x;}