This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
Re: std::vector default default and move constructors
- From: François Dumont <frs dot dumont at gmail dot com>
- To: Jonathan Wakely <jwakely at redhat dot com>
- Cc: libstdc++ at gcc dot gnu dot org, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 27 Jun 2018 22:26:56 +0200
- Subject: Re: std::vector default default and move constructors
- References: <2b48d135-7b0f-f8ec-73f5-15932c2a8581@gmail.com> <d4e95591-30ec-b366-d79e-df8416d44ef1@gmail.com> <1653b9da-8fee-d534-0185-3a63a69d5ca1@gmail.com> <2b6cd500-135a-4d23-9cfd-e064efabdfa7@gmail.com> <alpine.DEB.2.21.1801110748090.5897@stedding.saclay.inria.fr> <48c96890-91fa-ac4e-9df8-452139987955@gmail.com> <20180115122914.GJ24454@redhat.com> <fa620c68-ad59-ff66-40b9-e014d655cebe@gmail.com> <20180116002054.GL24454@redhat.com> <9a050bcd-ce7a-7342-e31e-bbb9b4ca72dc@gmail.com> <20180626134619.GZ2838@redhat.com>
Commited attached patch.
It fixes the missing noexcept qualification on a __gnu_debug::vector<>
constructor.
2018-06-27 François Dumont <fdumont@gcc.gnu.org>
* include/bits/stl_vector.h
(struct _Vector_base<>::_Vector_impl_data): New.
(struct _Vector_base<>::_Vector_impl): Inherit from latter.
(_Vector_base<>::_Vector_impl::_M_swap_data): Move...
(_Vector_base<>::_Vector_impl_data::_M_swap_data): ...here.
(_Vector_base<>::_Vector_impl()): Add noexcept qualification.
(_Vector_base<>::_Vector_impl(_Vector_impl&&)): New.
(_Vector_base<>::_Vector_impl(_Tp_alloc_type&&, _Vector_impl&&)): New.
(_Vector_base(const allocator_type&, _Vector_base&&)): New, use latter.
(_Vector_base()): Default.
(_Vector_base(_Vector_base&&)): Default.
(_Vector_base(size_t)) [_GLIBCXX_INLINE_VERSION]: Delete.
(_Vector_base(_Tp_alloc_type&&)) [_GLIBCXX_INLINE_VERSION]: Delete.
(_Vector_base::_M_create_storage(size_t)): Make protected.
(vector()): Default.
(vector(vector&&)): Default.
(vector(vector&&, const allocator_type&, true_type)): New.
(vector(vector&&, const allocator_type&, false_type)): New.
(vector(vector&&, const allocator_type&)): Use latters.
(vector(_InputIte, _InputIte, const allocator_type&)): Call
_M_range_initialize directly.
* include/debug/vector
(vector(vector&&, const allocator_type&)): Add noexcept qualification.
* testsuite/23_containers/vector/allocator/default_init.cc: New.
* testsuite/23_containers/vector/cons/noexcept_move_construct.cc: Add
static assertions.
On 26/06/2018 15:46, Jonathan Wakely wrote:
On 02/06/18 14:00 +0200, François Dumont wrote:
Hi
Here is this patch again, I consider all your remarks and also
made some changes considering feedback on rbtree patch.
+ _Vector_impl(_Tp_alloc_type const& __a) _GLIBCXX_NOEXCEPT
+ : _Tp_alloc_type(__a)
+ { }
+
+#if __cplusplus >= 201103L
+ // Not defaulted to avoid noexcept qualification dependency on the
+ // _Tp_alloc_type move constructor one.
Could you please rephrase this comment as:
// Not defaulted, to enforce noexcept(true) even when
// !is_nothrow_move_constructible<_Tp_alloc_type>.
I prefer this wording, because most allocators don't have a move
constructor at all (just a copy constructor) so talking about its move
constructor is misleading.
+ _Vector_impl(_Vector_impl&& __x) noexcept
+ : _Tp_alloc_type(std::move(__x)), _Vector_impl_data(std::move(__x))
+ { }
+
+ _Vector_impl(_Tp_alloc_type&& __a) noexcept
+ : _Tp_alloc_type(std::move(__a))
+ { }
+
+ _Vector_impl(_Tp_alloc_type&& __a, _Vector_impl&& __rv) noexcept
+ : _Tp_alloc_type(std::move(__a)),
_Vector_impl_data(std::move(__rv))
+ { }
+#endif
#if _GLIBCXX_SANITIZE_STD_ALLOCATOR && _GLIBCXX_SANITIZE_VECTOR
template<typename = _Tp_alloc_type>
@@ -235,38 +259,42 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_Tp_alloc_type&
_M_get_Tp_allocator() _GLIBCXX_NOEXCEPT
- { return *static_cast<_Tp_alloc_type*>(&this->_M_impl); }
+ { return this->_M_impl; }
const _Tp_alloc_type&
_M_get_Tp_allocator() const _GLIBCXX_NOEXCEPT
- { return *static_cast<const _Tp_alloc_type*>(&this->_M_impl); }
+ { return this->_M_impl; }
allocator_type
get_allocator() const _GLIBCXX_NOEXCEPT
{ return allocator_type(_M_get_Tp_allocator()); }
- _Vector_base()
- : _M_impl() { }
+#if __cplusplus >= 201103L
+ _Vector_base() = default;
+#else
+ _Vector_base() { }
+#endif
_Vector_base(const allocator_type& __a) _GLIBCXX_NOEXCEPT
: _M_impl(__a) { }
Please add "// Kept for ABI compatibility" before this #if:
+#if !_GLIBCXX_INLINE_VERSION
_Vector_base(size_t __n)
: _M_impl()
{ _M_create_storage(__n); }
+#endif
_Vector_base(size_t __n, const allocator_type& __a)
: _M_impl(__a)
{ _M_create_storage(__n); }
#if __cplusplus >= 201103L
+ _Vector_base(_Vector_base&&) = default;
+
And here too:
+# if !_GLIBCXX_INLINE_VERSION
_Vector_base(_Tp_alloc_type&& __a) noexcept
: _M_impl(std::move(__a)) { }
OK for trunk with those three comment changes.
Thanks for your patience waiting for the review.
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index acec501..76829fa 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -85,34 +85,58 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
typedef typename __gnu_cxx::__alloc_traits<_Tp_alloc_type>::pointer
pointer;
- struct _Vector_impl
- : public _Tp_alloc_type
+ struct _Vector_impl_data
{
pointer _M_start;
pointer _M_finish;
pointer _M_end_of_storage;
- _Vector_impl()
- : _Tp_alloc_type(), _M_start(), _M_finish(), _M_end_of_storage()
- { }
-
- _Vector_impl(_Tp_alloc_type const& __a) _GLIBCXX_NOEXCEPT
- : _Tp_alloc_type(__a), _M_start(), _M_finish(), _M_end_of_storage()
+ _Vector_impl_data() _GLIBCXX_NOEXCEPT
+ : _M_start(), _M_finish(), _M_end_of_storage()
{ }
#if __cplusplus >= 201103L
- _Vector_impl(_Tp_alloc_type&& __a) noexcept
- : _Tp_alloc_type(std::move(__a)),
- _M_start(), _M_finish(), _M_end_of_storage()
- { }
+ _Vector_impl_data(_Vector_impl_data&& __x) noexcept
+ : _M_start(__x._M_start), _M_finish(__x._M_finish),
+ _M_end_of_storage(__x._M_end_of_storage)
+ { __x._M_start = __x._M_finish = __x._M_end_of_storage = pointer(); }
#endif
- void _M_swap_data(_Vector_impl& __x) _GLIBCXX_NOEXCEPT
+ void
+ _M_swap_data(_Vector_impl_data& __x) _GLIBCXX_NOEXCEPT
{
std::swap(_M_start, __x._M_start);
std::swap(_M_finish, __x._M_finish);
std::swap(_M_end_of_storage, __x._M_end_of_storage);
}
+ };
+
+ struct _Vector_impl
+ : public _Tp_alloc_type, public _Vector_impl_data
+ {
+ _Vector_impl() _GLIBCXX_NOEXCEPT_IF( noexcept(_Tp_alloc_type()) )
+ : _Tp_alloc_type()
+ { }
+
+ _Vector_impl(_Tp_alloc_type const& __a) _GLIBCXX_NOEXCEPT
+ : _Tp_alloc_type(__a)
+ { }
+
+#if __cplusplus >= 201103L
+ // Not defaulted, to enforce noexcept(true) even when
+ // !is_nothrow_move_constructible<_Tp_alloc_type>.
+ _Vector_impl(_Vector_impl&& __x) noexcept
+ : _Tp_alloc_type(std::move(__x)), _Vector_impl_data(std::move(__x))
+ { }
+
+ _Vector_impl(_Tp_alloc_type&& __a) noexcept
+ : _Tp_alloc_type(std::move(__a))
+ { }
+
+ _Vector_impl(_Tp_alloc_type&& __a, _Vector_impl&& __rv) noexcept
+ : _Tp_alloc_type(std::move(__a)), _Vector_impl_data(std::move(__rv))
+ { }
+#endif
#if _GLIBCXX_SANITIZE_STD_ALLOCATOR && _GLIBCXX_SANITIZE_VECTOR
template<typename = _Tp_alloc_type>
@@ -235,38 +259,44 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_Tp_alloc_type&
_M_get_Tp_allocator() _GLIBCXX_NOEXCEPT
- { return *static_cast<_Tp_alloc_type*>(&this->_M_impl); }
+ { return this->_M_impl; }
const _Tp_alloc_type&
_M_get_Tp_allocator() const _GLIBCXX_NOEXCEPT
- { return *static_cast<const _Tp_alloc_type*>(&this->_M_impl); }
+ { return this->_M_impl; }
allocator_type
get_allocator() const _GLIBCXX_NOEXCEPT
{ return allocator_type(_M_get_Tp_allocator()); }
- _Vector_base()
- : _M_impl() { }
+#if __cplusplus >= 201103L
+ _Vector_base() = default;
+#else
+ _Vector_base() { }
+#endif
_Vector_base(const allocator_type& __a) _GLIBCXX_NOEXCEPT
: _M_impl(__a) { }
+ // Kept for ABI compatibility.
+#if !_GLIBCXX_INLINE_VERSION
_Vector_base(size_t __n)
: _M_impl()
{ _M_create_storage(__n); }
+#endif
_Vector_base(size_t __n, const allocator_type& __a)
: _M_impl(__a)
{ _M_create_storage(__n); }
#if __cplusplus >= 201103L
+ _Vector_base(_Vector_base&&) = default;
+
+ // Kept for ABI compatibility.
+# if !_GLIBCXX_INLINE_VERSION
_Vector_base(_Tp_alloc_type&& __a) noexcept
: _M_impl(std::move(__a)) { }
- _Vector_base(_Vector_base&& __x) noexcept
- : _M_impl(std::move(__x._M_get_Tp_allocator()))
- { this->_M_impl._M_swap_data(__x._M_impl); }
-
_Vector_base(_Vector_base&& __x, const allocator_type& __a)
: _M_impl(__a)
{
@@ -278,6 +308,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_M_create_storage(__n);
}
}
+# endif
+
+ _Vector_base(const allocator_type& __a, _Vector_base&& __x)
+ : _M_impl(_Tp_alloc_type(__a), std::move(__x._M_impl))
+ { }
#endif
~_Vector_base() _GLIBCXX_NOEXCEPT
@@ -304,7 +339,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_Tr::deallocate(_M_impl, __p, __n);
}
- private:
+ protected:
void
_M_create_storage(size_t __n)
{
@@ -388,11 +423,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
/**
* @brief Creates a %vector with no elements.
*/
- vector()
#if __cplusplus >= 201103L
- noexcept(is_nothrow_default_constructible<_Alloc>::value)
+ vector() = default;
+#else
+ vector() { }
#endif
- : _Base() { }
/**
* @brief Creates a %vector with no elements.
@@ -468,13 +503,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
#if __cplusplus >= 201103L
/**
* @brief %Vector move constructor.
- * @param __x A %vector of identical element and allocator types.
*
- * The newly-created %vector contains the exact contents of @a __x.
- * The contents of @a __x are a valid, but unspecified %vector.
+ * The newly-created %vector contains the exact contents of the
+ * moved instance.
+ * The contents of the moved instance are a valid, but unspecified
+ * %vector.
*/
- vector(vector&& __x) noexcept
- : _Base(std::move(__x)) { }
+ vector(vector&&) noexcept = default;
/// Copy constructor with alternative allocator
vector(const vector& __x, const allocator_type& __a)
@@ -486,13 +521,19 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_M_get_Tp_allocator());
}
- /// Move constructor with alternative allocator
- vector(vector&& __rv, const allocator_type& __m)
- noexcept(_Alloc_traits::_S_always_equal())
- : _Base(std::move(__rv), __m)
+ private:
+ vector(vector&& __rv, const allocator_type& __m, true_type) noexcept
+ : _Base(__m, std::move(__rv))
+ { }
+
+ vector(vector&& __rv, const allocator_type& __m, false_type)
+ : _Base(__m)
{
- if (__rv.get_allocator() != __m)
+ if (__rv.get_allocator() == __m)
+ this->_M_impl._M_swap_data(__rv._M_impl);
+ else if (!__rv.empty())
{
+ this->_M_create_storage(__rv.size());
this->_M_impl._M_finish =
std::__uninitialized_move_a(__rv.begin(), __rv.end(),
this->_M_impl._M_start,
@@ -501,6 +542,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
}
}
+ public:
+ /// Move constructor with alternative allocator
+ vector(vector&& __rv, const allocator_type& __m)
+ noexcept( noexcept(
+ vector(std::declval<vector&&>(), std::declval<const allocator_type&>(),
+ std::declval<typename _Alloc_traits::is_always_equal>())) )
+ : vector(std::move(__rv), __m, typename _Alloc_traits::is_always_equal{})
+ { }
+
/**
* @brief Builds a %vector from an initializer list.
* @param __l An initializer_list.
@@ -543,7 +593,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector(_InputIterator __first, _InputIterator __last,
const allocator_type& __a = allocator_type())
: _Base(__a)
- { _M_initialize_dispatch(__first, __last, __false_type()); }
+ {
+ _M_range_initialize(__first, __last,
+ std::__iterator_category(__first));
+ }
#else
template<typename _InputIterator>
vector(_InputIterator __first, _InputIterator __last,
@@ -1414,6 +1467,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
// Called by the range constructor to implement [23.1.1]/9
+#if __cplusplus < 201103L
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// 438. Ambiguity in the "do the right thing" clause
template<typename _Integer>
@@ -1432,10 +1486,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_M_initialize_dispatch(_InputIterator __first, _InputIterator __last,
__false_type)
{
- typedef typename std::iterator_traits<_InputIterator>::
- iterator_category _IterCategory;
- _M_range_initialize(__first, __last, _IterCategory());
+ _M_range_initialize(__first, __last,
+ std::__iterator_category(__first));
}
+#endif
// Called by the second initialize_dispatch above
template<typename _InputIterator>
@@ -1669,7 +1723,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
// moved, either because the source's allocator will move too
// or because the allocators are equal.
void
- _M_move_assign(vector&& __x, std::true_type) noexcept
+ _M_move_assign(vector&& __x, true_type) noexcept
{
vector __tmp(get_allocator());
this->_M_impl._M_swap_data(__tmp._M_impl);
@@ -1680,10 +1734,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
// Do move assignment when it might not be possible to move source
// object's memory, resulting in a linear-time operation.
void
- _M_move_assign(vector&& __x, std::false_type)
+ _M_move_assign(vector&& __x, false_type)
{
if (__x._M_get_Tp_allocator() == this->_M_get_Tp_allocator())
- _M_move_assign(std::move(__x), std::true_type());
+ _M_move_assign(std::move(__x), true_type());
else
{
// The rvalue's allocator cannot be moved and is not equal,
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index 8d60da3..802f4fd 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -199,6 +199,8 @@ namespace __debug
: _Base(__x, __a) { }
vector(vector&& __x, const allocator_type& __a)
+ noexcept( noexcept(
+ _Base(std::declval<_Base&&>()), std::declval<const allocator_type&>()) )
: _Safe(std::move(__x._M_safe()), __a),
_Base(std::move(__x._M_base()), __a),
_Safe_vector(std::move(__x)) { }
diff --git a/libstdc++-v3/testsuite/23_containers/vector/allocator/default_init.cc b/libstdc++-v3/testsuite/23_containers/vector/allocator/default_init.cc
new file mode 100644
index 0000000..5981b8f
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/allocator/default_init.cc
@@ -0,0 +1,67 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+// { dg-options "-O0" }
+// { dg-xfail-run-if "PR c++/65816" { *-*-* } }
+
+#include <vector>
+#include <testsuite_hooks.h>
+#include <testsuite_allocator.h>
+
+#include <ext/aligned_buffer.h>
+
+using T = int;
+
+using __gnu_test::default_init_allocator;
+
+void test01()
+{
+ typedef default_init_allocator<T> alloc_type;
+ typedef std::vector<T, alloc_type> test_type;
+
+ __gnu_cxx::__aligned_buffer<test_type> buf;
+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
+
+ test_type *tmp = ::new(buf._M_addr()) test_type;
+
+ VERIFY( tmp->get_allocator().state == 0 );
+
+ tmp->~test_type();
+}
+
+void test02()
+{
+ typedef default_init_allocator<T> alloc_type;
+ typedef std::vector<T, alloc_type> test_type;
+
+ __gnu_cxx::__aligned_buffer<test_type> buf;
+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
+
+ test_type *tmp = ::new(buf._M_addr()) test_type();
+
+ VERIFY( tmp->get_allocator().state == 0 );
+
+ tmp->~test_type();
+}
+
+int main()
+{
+ test01();
+ test02();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/vector/cons/noexcept_move_construct.cc b/libstdc++-v3/testsuite/23_containers/vector/cons/noexcept_move_construct.cc
index f8f71a5..4d41486 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/cons/noexcept_move_construct.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/cons/noexcept_move_construct.cc
@@ -23,4 +23,34 @@
typedef std::vector<int> vtype;
-static_assert(std::is_nothrow_move_constructible<vtype>::value, "Error");
+static_assert( std::is_nothrow_move_constructible<vtype>::value,
+ "noexcept move constructor" );
+static_assert( std::is_nothrow_constructible<vtype,
+ vtype&&, const typename vtype::allocator_type&>::value,
+ "noexcept move constructor with allocator" );
+
+template<typename Type>
+ class not_noexcept_move_constructor_alloc : public std::allocator<Type>
+ {
+ public:
+ not_noexcept_move_constructor_alloc() noexcept { }
+
+ not_noexcept_move_constructor_alloc(
+ const not_noexcept_move_constructor_alloc& x) noexcept
+ : std::allocator<Type>(x)
+ { }
+
+ not_noexcept_move_constructor_alloc(
+ not_noexcept_move_constructor_alloc&& x) noexcept(false)
+ : std::allocator<Type>(std::move(x))
+ { }
+
+ template<typename _Tp1>
+ struct rebind
+ { typedef not_noexcept_move_constructor_alloc<_Tp1> other; };
+ };
+
+typedef std::vector<int, not_noexcept_move_constructor_alloc<int>> vtype2;
+
+static_assert( std::is_nothrow_move_constructible<vtype2>::value,
+ "noexcept move constructor with not noexcept alloc" );