[libcxx] r334053 - Fix PR37694 - std::vector doesn't correctly move construct allocators.
Eric Fiselier via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 5 15:32:52 PDT 2018
Author: ericwf
Date: Tue Jun 5 15:32:52 2018
New Revision: 334053
URL: http://llvm.org/viewvc/llvm-project?rev=334053&view=rev
Log:
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.
Added:
libcxx/trunk/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp
Modified:
libcxx/trunk/include/forward_list
libcxx/trunk/include/list
libcxx/trunk/include/vector
libcxx/trunk/test/std/containers/sequences/vector.bool/move.pass.cpp
libcxx/trunk/test/std/containers/sequences/vector/vector.cons/move.pass.cpp
libcxx/trunk/test/support/test_allocator.h
Modified: libcxx/trunk/include/forward_list
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/forward_list?rev=334053&r1=334052&r2=334053&view=diff
==============================================================================
--- libcxx/trunk/include/forward_list (original)
+++ libcxx/trunk/include/forward_list Tue Jun 5 15:32:52 2018
@@ -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,12 +960,9 @@ forward_list<_Tp, _Alloc>::forward_list(
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())
- )
- )
-{
- insert_after(cbefore_begin(), __x.begin(), __x.end());
+ : base(
+ __node_traits::select_on_container_copy_construction(__x.__alloc())) {
+ insert_after(cbefore_begin(), __x.begin(), __x.end());
}
template <class _Tp, class _Alloc>
Modified: libcxx/trunk/include/list
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/list?rev=334053&r1=334052&r2=334053&view=diff
==============================================================================
--- libcxx/trunk/include/list (original)
+++ libcxx/trunk/include/list Tue Jun 5 15:32:52 2018
@@ -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,9 +721,18 @@ __list_imp<_Tp, _Alloc>::__list_imp(cons
}
template <class _Tp, class _Alloc>
-__list_imp<_Tp, _Alloc>::~__list_imp()
-{
- clear();
+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);
#endif
@@ -1248,10 +1265,8 @@ list<_Tp, _Alloc>::list(_InpIter __f, _I
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
}
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
Modified: libcxx/trunk/include/vector
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/vector?rev=334053&r1=334052&r2=334053&view=diff
==============================================================================
--- libcxx/trunk/include/vector (original)
+++ libcxx/trunk/include/vector Tue Jun 5 15:32:52 2018
@@ -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
{
}
+#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,17 +2875,15 @@ vector<bool, _Allocator>::operator=(cons
#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
+ _NOEXCEPT
#else
- _NOEXCEPT_(is_nothrow_move_constructible<allocator_type>::value)
+ _NOEXCEPT_(is_nothrow_move_constructible<allocator_type>::value)
#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;
Added: libcxx/trunk/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp?rev=334053&view=auto
==============================================================================
--- libcxx/trunk/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp (added)
+++ libcxx/trunk/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp Tue Jun 5 15:32:52 2018
@@ -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);
+ }
+}
Modified: libcxx/trunk/test/std/containers/sequences/vector.bool/move.pass.cpp
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/containers/sequences/vector.bool/move.pass.cpp?rev=334053&r1=334052&r2=334053&view=diff
==============================================================================
--- libcxx/trunk/test/std/containers/sequences/vector.bool/move.pass.cpp (original)
+++ libcxx/trunk/test/std/containers/sequences/vector.bool/move.pass.cpp Tue Jun 5 15:32:52 2018
@@ -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);
+ }
+ }
}
Modified: libcxx/trunk/test/std/containers/sequences/vector/vector.cons/move.pass.cpp
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/containers/sequences/vector/vector.cons/move.pass.cpp?rev=334053&r1=334052&r2=334053&view=diff
==============================================================================
--- libcxx/trunk/test/std/containers/sequences/vector/vector.cons/move.pass.cpp (original)
+++ libcxx/trunk/test/std/containers/sequences/vector/vector.cons/move.pass.cpp Tue Jun 5 15:32:52 2018
@@ -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);
+ }
+ }
}
Modified: libcxx/trunk/test/support/test_allocator.h
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/support/test_allocator.h?rev=334053&r1=334052&r2=334053&view=diff
==============================================================================
--- libcxx/trunk/test/support/test_allocator.h (original)
+++ libcxx/trunk/test/support/test_allocator.h Tue Jun 5 15:32:52 2018
@@ -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;}
More information about the cfe-commits
mailing list