Forward list default default and move constructors

Forward list default default and move constructors

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

Hi

     Here is the patch to default the default and move constructors on 
the std::forward_list. Putting a move constructor on _Fwd_list_node_base 
helped limiting the code impact of this patch. It doesn't have any side 
effect as iterator types using this base type are not defining any move 
semantic.

     I also took the time to optimize the move constructor with 
allocator when allocator is always equal. It avoids initializing an 
empty forward list for nothing.

     I think it is fine but could we have an abi issue because of the 
change in forward_list.tcc ?

     * include/bits/forward_list.h
     (_Fwd_list_node_base(_Fwd_list_node_base&&)): New.
     (_Fwd_list_impl()): Add noexcept qualification.
     (_Fwd_list_impl(_Fwd_list_impl&&)): New, default.
     (_Fwd_list_impl(_Fwd_list_impl&&, _Node_alloc_type&&)): New.
     (_Fwd_list_base()): Default.
     (_Fwd_list_base(_Fwd_list_base&&, _Node_alloc_type&&, true_type)): New.
     (_Fwd_list_base(_Fwd_list_base&&, _Node_alloc_type&&, false_type)): 
New.
     (_Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a)): Use
     latter.
     (_Fwd_list_base(_Fwd_list_base&&)): Default.
     (forward_list<>()): Default.
     (forward_list<>(forward_list&&)): Default.
     * include/bits/forward_list.tcc
     (_Fwd_list_base(_Fwd_list_base&&, _Node_alloc_type&&, false_type)): 
New.
     * testsuite/23_containers/forward_list/allocator/default_init.cc: New.

Tested under Linux x86_64, ok to commit ?

François


[-- Attachment #2: forward_list.h.patch --]
[-- Type: text/x-patch, Size: 6500 bytes --]

diff --git a/libstdc++-v3/include/bits/forward_list.h b/libstdc++-v3/include/bits/forward_list.h
index f319b7f..312cd9e 100644
--- a/libstdc++-v3/include/bits/forward_list.h
+++ b/libstdc++-v3/include/bits/forward_list.h
@@ -53,6 +53,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
   struct _Fwd_list_node_base
   {
     _Fwd_list_node_base() = default;
+    _Fwd_list_node_base(_Fwd_list_node_base&& __x) noexcept
+      : _M_next(__x._M_next)
+    { __x._M_next = nullptr; }
 
     _Fwd_list_node_base* _M_next = nullptr;
 
@@ -284,15 +287,22 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
         _Fwd_list_node_base _M_head;
 
 	_Fwd_list_impl()
-        : _Node_alloc_type(), _M_head()
+	  noexcept( noexcept(_Node_alloc_type()) )
+	: _Node_alloc_type()
 	{ }
 
 	_Fwd_list_impl(const _Node_alloc_type& __a)
-        : _Node_alloc_type(__a), _M_head()
+	: _Node_alloc_type(__a)
+	{ }
+
+	_Fwd_list_impl(_Fwd_list_impl&&) = default;
+
+	_Fwd_list_impl(_Fwd_list_impl&& __fl, _Node_alloc_type&& __a)
+	: _Node_alloc_type(std::move(__a)), _M_head(std::move(__fl._M_head))
 	{ }
 
 	_Fwd_list_impl(_Node_alloc_type&& __a)
-	: _Node_alloc_type(std::move(__a)), _M_head()
+	: _Node_alloc_type(std::move(__a))
 	{ }
       };
 
@@ -311,20 +321,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _M_get_Node_allocator() const noexcept
       { return this->_M_impl; }
 
-      _Fwd_list_base()
-      : _M_impl() { }
+    private:
+      _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a, std::true_type)
+	: _M_impl(std::move(__lst._M_impl), std::move(__a))
+      { }
+
+      _Fwd_list_base(_Fwd_list_base&&, _Node_alloc_type&&, std::false_type);
+
+    public:
+      _Fwd_list_base() = default;
 
       _Fwd_list_base(_Node_alloc_type&& __a)
       : _M_impl(std::move(__a)) { }
 
-      _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a);
+      _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a)
+      : _Fwd_list_base(std::move(__lst), std::move(__a),
+		       typename _Node_alloc_traits::is_always_equal{})
+      { }
 
-      _Fwd_list_base(_Fwd_list_base&& __lst)
-      : _M_impl(std::move(__lst._M_get_Node_allocator()))
-      {
-	this->_M_impl._M_head._M_next = __lst._M_impl._M_head._M_next;
-	__lst._M_impl._M_head._M_next = 0;
-      }
+      _Fwd_list_base(_Fwd_list_base&&) = default;
 
       ~_Fwd_list_base()
       { _M_erase_after(&_M_impl._M_head, 0); }
@@ -436,10 +451,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       /**
        *  @brief  Creates a %forward_list with no elements.
        */
-      forward_list()
-      noexcept(is_nothrow_default_constructible<_Node_alloc_type>::value)
-      : _Base()
-      { }
+      forward_list() = default;
 
       /**
        *  @brief  Creates a %forward_list with no elements.
@@ -532,15 +544,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
       /**
        *  @brief  The %forward_list move constructor.
-       *  @param  __list  A %forward_list of identical element and allocator
-       *                  types.
+       *  @param  A %forward_list of identical element and allocator types.
        *
-       *  The newly-created %forward_list contains the exact contents of @a
-       *  __list. The contents of @a __list are a valid, but unspecified
-       *  %forward_list.
+       *  The newly-created %forward_list contains the exact contents of the
+       *  moved instance. The content of the moved instance is valid, but
+       *  unspecified %forward_list.
        */
-      forward_list(forward_list&& __list) noexcept
-      : _Base(std::move(__list)) { }
+      forward_list(forward_list&&) = default;
 
       /**
        *  @brief  Builds a %forward_list from an initializer_list
diff --git a/libstdc++-v3/include/bits/forward_list.tcc b/libstdc++-v3/include/bits/forward_list.tcc
index b823b09..95db8a6 100644
--- a/libstdc++-v3/include/bits/forward_list.tcc
+++ b/libstdc++-v3/include/bits/forward_list.tcc
@@ -36,7 +36,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
   template<typename _Tp, typename _Alloc>
     _Fwd_list_base<_Tp, _Alloc>::
-    _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a)
+    _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a,
+		   std::false_type)
     : _M_impl(std::move(__a))
     {
       if (__lst._M_get_Node_allocator() == _M_get_Node_allocator())
diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/allocator/default_init.cc b/libstdc++-v3/testsuite/23_containers/forward_list/allocator/default_init.cc
new file mode 100644
index 0000000..b56c9cc
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/forward_list/allocator/default_init.cc
@@ -0,0 +1,67 @@
+// Copyright (C) 2017 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 <forward_list>
+#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::forward_list<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::forward_list<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;
+}

Re: Forward list default default and move constructors

[Jonathan Wakely]

On 19/06/17 22:48 +0200, François Dumont wrote:
>Hi
>
>    Here is the patch to default the default and move constructors on 
>the std::forward_list. Putting a move constructor on 
>_Fwd_list_node_base helped limiting the code impact of this patch. It 
>doesn't have any side effect as iterator types using this base type 
>are not defining any move semantic.

I don't understand this comment.

1) The iterators only _Fwd_list_node_base* pointers, so that's why
they aren't affected. It's not because the iterators don't define move
semantics.

2) The iterators *do* have move semantics, they have
implicitly-declared move operations, which are identical to their
implicitly-defined copy operations (because moving a pointer is
identical to copying it).

3) Adding this move constructor has a pretty large side effect because
now its copy constructor and copy assignment operator are defined as
deleted, and it has no move assignment operator. That's OK, because we
never copy or move nodes (except in the new _Fwd_list_impl move ctor
you're adding). But it's a significant side effect. Please consider
adding the following to make those side effects explicit:

  _Fwd_list_node_base(const _Fwd_list_node_base&) = delete;
  _Fwd_list_node_base& operator=(const _Fwd_list_node_base&) = delete;
  _Fwd_list_node_base& operator=(_Fwd_list_node_base&&) = delete;


>    I also took the time to optimize the move constructor with 
>allocator when allocator is always equal. It avoids initializing an 
>empty forward list for nothing.
>
>    I think it is fine but could we have an abi issue because of the 
>change in forward_list.tcc ?

Old code with undefined references to that constructor will still find
a definition in new code that explicitly instantiates a forward_list.

New code compiled after your change would not find the new
constructors (the ones with true_type and false_type parameters) in
old code that explicitly instantiated a forward_list.

Could you split that part of the change into a separate patch? The
changes to define constructors as defaulted are OK, so I'd like to
considere the proposed optimisation separately.


 

Re: Forward list default default and move constructors

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 2508 bytes --]

On 05/07/2017 18:10, Jonathan Wakely wrote:
> On 19/06/17 22:48 +0200, François Dumont wrote:
>> Hi
>>
>>    Here is the patch to default the default and move constructors on 
>> the std::forward_list. Putting a move constructor on 
>> _Fwd_list_node_base helped limiting the code impact of this patch. It 
>> doesn't have any side effect as iterator types using this base type 
>> are not defining any move semantic.
>
> I don't understand this comment.
>
> 1) The iterators only _Fwd_list_node_base* pointers, so that's why
> they aren't affected. It's not because the iterators don't define move
> semantics.
>
> 2) The iterators *do* have move semantics, they have
> implicitly-declared move operations, which are identical to their
> implicitly-defined copy operations (because moving a pointer is
> identical to copying it).
>
> 3) Adding this move constructor has a pretty large side effect because
> now its copy constructor and copy assignment operator are defined as
> deleted, and it has no move assignment operator. That's OK, because we
> never copy or move nodes (except in the new _Fwd_list_impl move ctor
> you're adding). But it's a significant side effect. Please consider
> adding the following to make those side effects explicit:
>
>  _Fwd_list_node_base(const _Fwd_list_node_base&) = delete;
>  _Fwd_list_node_base& operator=(const _Fwd_list_node_base&) = delete;
>  _Fwd_list_node_base& operator=(_Fwd_list_node_base&&) = delete;

Yes, sorry, my comment was indeed wrong. I should have limit it to say 
that I was not seeing real side effect considering current code. I added 
those explicit special deleted members.

>
>
>>    I also took the time to optimize the move constructor with 
>> allocator when allocator is always equal. It avoids initializing an 
>> empty forward list for nothing.
>>
>>    I think it is fine but could we have an abi issue because of the 
>> change in forward_list.tcc ?
>
> Old code with undefined references to that constructor will still find
> a definition in new code that explicitly instantiates a forward_list.
>
> New code compiled after your change would not find the new
> constructors (the ones with true_type and false_type parameters) in
> old code that explicitly instantiated a forward_list.
>
> Could you split that part of the change into a separate patch? The
> changes to define constructors as defaulted are OK, so I'd like to
> considere the proposed optimisation separately 

Done in attached patch, ok to commit ?

François


[-- Attachment #2: forward_list.h.patch --]
[-- Type: text/x-patch, Size: 5610 bytes --]

diff --git a/libstdc++-v3/include/bits/forward_list.h b/libstdc++-v3/include/bits/forward_list.h
index f319b7f..6a0e54b 100644
--- a/libstdc++-v3/include/bits/forward_list.h
+++ b/libstdc++-v3/include/bits/forward_list.h
@@ -53,6 +53,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
   struct _Fwd_list_node_base
   {
     _Fwd_list_node_base() = default;
+    _Fwd_list_node_base(_Fwd_list_node_base&& __x) noexcept
+      : _M_next(__x._M_next)
+    { __x._M_next = nullptr; }
+
+    _Fwd_list_node_base(const _Fwd_list_node_base&) = delete;
+    _Fwd_list_node_base& operator=(const _Fwd_list_node_base&) = delete;
+    _Fwd_list_node_base& operator=(_Fwd_list_node_base&&) = delete;
 
     _Fwd_list_node_base* _M_next = nullptr;
 
@@ -283,17 +290,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       {
         _Fwd_list_node_base _M_head;
 
-        _Fwd_list_impl()
-        : _Node_alloc_type(), _M_head()
-        { }
+	_Fwd_list_impl()
+	  noexcept( noexcept(_Node_alloc_type()) )
+	: _Node_alloc_type()
+	{ }
 
-        _Fwd_list_impl(const _Node_alloc_type& __a)
-        : _Node_alloc_type(__a), _M_head()
-        { }
+	_Fwd_list_impl(_Fwd_list_impl&&) = default;
 
-        _Fwd_list_impl(_Node_alloc_type&& __a)
-	: _Node_alloc_type(std::move(__a)), _M_head()
-        { }
+	_Fwd_list_impl(_Node_alloc_type&& __a)
+	: _Node_alloc_type(std::move(__a))
+	{ }
       };
 
       _Fwd_list_impl _M_impl;
@@ -311,20 +317,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _M_get_Node_allocator() const noexcept
       { return this->_M_impl; }
 
-      _Fwd_list_base()
-      : _M_impl() { }
+      _Fwd_list_base() = default;
 
       _Fwd_list_base(_Node_alloc_type&& __a)
       : _M_impl(std::move(__a)) { }
 
       _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a);
 
-      _Fwd_list_base(_Fwd_list_base&& __lst)
-      : _M_impl(std::move(__lst._M_get_Node_allocator()))
-      {
-	this->_M_impl._M_head._M_next = __lst._M_impl._M_head._M_next;
-	__lst._M_impl._M_head._M_next = 0;
-      }
+      _Fwd_list_base(_Fwd_list_base&&) = default;
 
       ~_Fwd_list_base()
       { _M_erase_after(&_M_impl._M_head, 0); }
@@ -436,10 +436,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       /**
        *  @brief  Creates a %forward_list with no elements.
        */
-      forward_list()
-      noexcept(is_nothrow_default_constructible<_Node_alloc_type>::value)
-      : _Base()
-      { }
+      forward_list() = default;
 
       /**
        *  @brief  Creates a %forward_list with no elements.
@@ -532,15 +529,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
       /**
        *  @brief  The %forward_list move constructor.
-       *  @param  __list  A %forward_list of identical element and allocator
-       *                  types.
+       *  @param  A %forward_list of identical element and allocator types.
        *
-       *  The newly-created %forward_list contains the exact contents of @a
-       *  __list. The contents of @a __list are a valid, but unspecified
-       *  %forward_list.
+       *  The newly-created %forward_list contains the exact contents of the
+       *  moved instance. The contents of the moved instance are a valid, but
+       *  unspecified %forward_list.
        */
-      forward_list(forward_list&& __list) noexcept
-      : _Base(std::move(__list)) { }
+      forward_list(forward_list&&) = default;
 
       /**
        *  @brief  Builds a %forward_list from an initializer_list
diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/allocator/default_init.cc b/libstdc++-v3/testsuite/23_containers/forward_list/allocator/default_init.cc
new file mode 100644
index 0000000..b56c9cc
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/forward_list/allocator/default_init.cc
@@ -0,0 +1,67 @@
+// Copyright (C) 2017 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 <forward_list>
+#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::forward_list<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::forward_list<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;
+}