Re: Forward list default default and move constructors

[François Dumont <frs dot dumont at gmail dot com>]

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

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;
+}