This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
Re: Default std::list default and move constructors
- From: Jonathan Wakely <jwakely at redhat dot com>
- To: François Dumont <frs dot dumont at gmail dot com>
- Cc: "libstdc++ at gcc dot gnu dot org" <libstdc++ at gcc dot gnu dot org>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 5 Jul 2017 16:22:54 +0100
- Subject: Re: Default std::list default and move constructors
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jwakely at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 7C2C385A05
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7C2C385A05
- References: <acb5bb2a-d98d-1aa7-b58e-c319bcf52869@gmail.com>
On 26/06/17 21:29 +0200, François Dumont wrote:
Hi
Here is the patch to default implementation of std::list default
and move constructors.
I introduce _List_node_header to take care of the move
implementation and also isolate management of the optional list size
storage. I prefer it to usage of _List_node<size_t> as move
constructor seems complicated to implement with an __aligned_membuf.
It also avoids to use raw memory as-if it was a size_t without
constructing it. Even if size_t constructor is trivial I guess some
memory analyser could have complain about it.
No, that's not something we should be concerned about. As long as we
write to that memory before we read it, there's no problem. I still
like your new _List_node_header type, but not for this reason.
* include/bits/stl_list.h
(_List_node_base()): Define.
(_List_node_base(_List_node_base*, _List_node_base*)): New.
(struct _List_node_header): New.
(_List_impl()): Fix noexcept qualification.
(_List_impl(_List_impl&&)): New, default.
(_List_impl(_List_impl&&, _Node_alloc_type&&)): New.
(_List_base()): Default.
(_List_base(_List_base&&)): Default.
(_List_base(_List_base&&, _Node_alloc_type&&, true_type)): New.
(_List_base(_List_base&&, _Node_alloc_type&&, false_type)): New.
(_List_base(_List_base&&, _Node_alloc_type&&)): Use latters.
(_List_base::_M_move_nodes): Adapt to use
_List_node_header._M_move_nodes.
(_List_base::_M_init): Likewise.
(list<>()): Default.
(list<>(list&&)): Default.
(list<>::_M_move_assign(list&&, true_type)): Use _M_move_nodes.
Tested under Linux x86_64.
Ok to commit ?
It's mostly good, but I'd like to make a few suggestions ...
diff --git a/libstdc++-v3/include/bits/stl_list.h b/libstdc++-v3/include/bits/stl_list.h
index 232885a..7ffffe5 100644
--- a/libstdc++-v3/include/bits/stl_list.h
+++ b/libstdc++-v3/include/bits/stl_list.h
@@ -82,6 +82,17 @@ namespace std _GLIBCXX_VISIBILITY(default)
_List_node_base* _M_next;
_List_node_base* _M_prev;
+#if __cplusplus >= 201103L
+ _List_node_base() = default;
+#else
+ _List_node_base()
+ { }
+#endif
+
+ _List_node_base(_List_node_base* __next, _List_node_base* __prev)
+ : _M_next(__next), _M_prev(__prev)
+ { }
+
I think I'd prefer to leave this struct with no user-defined
constructors, instead of adding these.
static void
swap(_List_node_base& __x, _List_node_base& __y) _GLIBCXX_USE_NOEXCEPT;
@@ -99,6 +110,79 @@ namespace std _GLIBCXX_VISIBILITY(default)
_M_unhook() _GLIBCXX_USE_NOEXCEPT;
};
+ /// The %list node header.
+ struct _List_node_header : public _List_node_base
+ {
+ private:
+#if _GLIBCXX_USE_CXX11_ABI
+ std::size_t _M_size;
+#endif
I don't think this needs to be private, because we don't have to worry
about users accessing this member. It's an internal-only type, and the
_M_next and _M_prev members are already public.
If it's public then the _List_base::_M_inc_size, _M_dec_size etc.
could access it directly, and we don't need to add duplicates of those
functions to _List_impl.
+
+ _List_node_base* _M_base() { return this; }
Is this function necessary?
+ public:
+ _List_node_header() _GLIBCXX_NOEXCEPT
+ : _List_node_base(this, this)
+# if _GLIBCXX_USE_CXX11_ABI
+ , _M_size(0)
+# endif
+ { }
This could be:
_List_node_header() _GLIBCXX_NOEXCEPT
{ _M_init(); }
+#if __cplusplus >= 201103L
+ _List_node_header(_List_node_header&& __x) noexcept
+ : _List_node_base(__x._M_next, __x._M_prev)
And this could use aggregate-initialization:
: _List_node_base{__x._M_next, __x._M_prev}
+# if _GLIBCXX_USE_CXX11_ABI
+ , _M_size(__x._M_size)
+# endif
+ {
+ if (__x._M_base()->_M_next == __x._M_base())
+ this->_M_next = this->_M_prev = this;
+ else
+ {
+ this->_M_next->_M_prev = this->_M_prev->_M_next = this->_M_base();
+ __x._M_init();
+ }
+ }
+#if _GLIBCXX_USE_CXX11_ABI
+ size_t _M_get_size() const { return _M_size; }
+ void _M_set_size(size_t __n) { _M_size = __n; }
+ void _M_inc_size(size_t __n) { _M_size += __n; }
+ void _M_dec_size(size_t __n) { _M_size -= __n; }
+#else
+ // dummy implementations used when the size is not stored
+ size_t _M_get_size() const { return 0; }
+ void _M_set_size(size_t) { }
+ void _M_inc_size(size_t) { }
+ void _M_dec_size(size_t) { }
+#endif
What do you think about only having _M_set_size() here?
The other functions are not needed here (assuming _M_size is public).
We could even get rid of _M_set_size and use #if in _M_init:
+ void
+ _M_init() _GLIBCXX_NOEXCEPT
+ {
+ this->_M_next = this->_M_prev = this;
#if _GLIBCXX_USE_CXX11_ABI
_M_size = 0;
#endif
+ _M_set_size(0);
+ }
+ };
This replaces a #if and #else and four functions with a #if and no
functions, which seems simpler to me.
@@ -323,51 +407,53 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
struct _List_impl
: public _Node_alloc_type
{
-#if _GLIBCXX_USE_CXX11_ABI
- _List_node<size_t> _M_node;
-#else
- __detail::_List_node_base _M_node;
-#endif
+ __detail::_List_node_header _M_node;
I agree that introducing the new header type helps, as this is
simpler and clearer, and so are the following constructors ...
- _List_impl() _GLIBCXX_NOEXCEPT
- : _Node_alloc_type(), _M_node()
+ _List_impl() _GLIBCXX_NOEXCEPT_IF( noexcept(_Node_alloc_type()) )
+ : _Node_alloc_type()
{ }
_List_impl(const _Node_alloc_type& __a) _GLIBCXX_NOEXCEPT
- : _Node_alloc_type(__a), _M_node()
+ : _Node_alloc_type(__a)
{ }
#if __cplusplus >= 201103L
+ _List_impl(_List_impl&&) = default;
+
+ _List_impl(_List_impl&& __x, _Node_alloc_type&& __a)
+ : _Node_alloc_type(std::move(__a)), _M_node(std::move(__x._M_node))
+ { }
+
_List_impl(_Node_alloc_type&& __a) noexcept
- : _Node_alloc_type(std::move(__a)), _M_node()
+ : _Node_alloc_type(std::move(__a))
{ }
#endif
};
_List_impl _M_impl;
-#if _GLIBCXX_USE_CXX11_ABI
- size_t _M_get_size() const { return *_M_impl._M_node._M_valptr(); }
+ size_t
+ _M_get_size() const { return _M_impl._M_node._M_get_size(); }
- void _M_set_size(size_t __n) { *_M_impl._M_node._M_valptr() = __n; }
+ void
+ _M_set_size(size_t __n) { _M_impl._M_node._M_set_size(__n); }
- void _M_inc_size(size_t __n) { *_M_impl._M_node._M_valptr() += __n; }
+ void
+ _M_inc_size(size_t __n) { _M_impl._M_node._M_inc_size(__n); }
- void _M_dec_size(size_t __n) { *_M_impl._M_node._M_valptr() -= __n; }
+ void
+ _M_dec_size(size_t __n) { _M_impl._M_node._M_dec_size(__n); }
These functions could just access _M_impl._M_size directly if it was
public. Introducing new functions to _List_impl to be used here seems
like unnecessary complication. We don't get rid of the #if because
it's still needed for these functions anyway:
+#if _GLIBCXX_USE_CXX11_ABI
size_t
_M_distance(const __detail::_List_node_base* __first,
const __detail::_List_node_base* __last) const
{ return _S_distance(__first, __last); }
// return the stored size
- size_t _M_node_count() const { return *_M_impl._M_node._M_valptr(); }
+ size_t _M_node_count() const { return _M_impl._M_node._M_get_size(); }
#else
// dummy implementations used when the size is not stored
- size_t _M_get_size() const { return 0; }
- void _M_set_size(size_t) { }
- void _M_inc_size(size_t) { }
- void _M_dec_size(size_t) { }
size_t _M_distance(const void*, const void*) const { return 0; }
// count the number of nodes
- else
- _M_init(); // Caller must move individual elements.
+ // else Caller must move individual elements.
s/Caller/caller/
@@ -657,13 +736,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
#if __cplusplus >= 201103L
/**
* @brief %List move constructor.
- * @param __x A %list of identical element and allocator types.
*
- * The newly-created %list contains the exact contents of @a __x.
- * The contents of @a __x are a valid, but unspecified %list.
+ * The newly-created %list contains the exact contents of the moved
+ * instance. The contents of @a __x are a valid, but unspecified %list.
The documentation still refers to __x but there's no longer a
parameter called __x.
*/
- list(list&& __x) noexcept
- : _Base(std::move(__x)) { }
+ list(list&&) = default;
/**
* @brief Builds a %list from an initializer_list