diff mbox

Forward list default default and move constructors

Message ID a1908d48-ac05-c7e6-a8e9-ef19f4bca3b7@gmail.com
State New
Headers show

Commit Message

François Dumont June 19, 2017, 8:48 p.m. UTC
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

Comments

Jonathan Wakely July 5, 2017, 4:10 p.m. UTC | #1
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.
diff mbox

Patch

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