Re: Default std::list default and move constructors

[Jonathan Wakely]

On 12/07/17 22:12 +0200, François Dumont wrote:

On 05/07/2017 17:22, Jonathan Wakely wrote:

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.

My goal was to make sure that as much as possible instances are initialized through their initialization list. But it is still possible without it so I made the change.

     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.

It's not internal-only as those members are exposed on std::list through inheritance. But I agree that consistency is more important here so I made it public.

Looks like it's only used as a base class of _List_impl, which is
private to std::list, and in the __distance overload. Am I missing
something?

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?

It is a nice replacement for calls to __addressof.

OK, that does make it more readable.

-#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:

Yes, I wanted to manage as much as possible usage of C++11 abi in the new _List_node_header type.

N.B. the ABI is called "cxx11" not C++11. It's a bad name, I should
have called it something else, but please don't make it worse by
saying "C++11". That ABI is also the default for C++98 so saying
"C++11" just causes more confusion than necessary.

You're not getting rid of the #if entirely, so some of the cxx11 ABI
specialization already lives in _List_base. Adding several new member
functions and still failing to remove the #if#else from _List_base
didn't seem like an improvement. The new patch looks much better to
me.

So here is the new patch limited to this evolution. Optimization for always equal allocator will come after along with another simplification to replace _S_distance with std::distance.

OK.

Tests running, ok to commit if successful ?

OK for trunk with one tiny tweak, while you're already changing the
function ...

@@ -1983,12 +2011,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
               _GLIBCXX_STD_C::_List_const_iterator<_Tp> __last,
               input_iterator_tag)
    {
-      typedef _GLIBCXX_STD_C::_List_node<size_t> _Sentinel;
+      typedef __detail::_List_node_header _Sentinel;
      _GLIBCXX_STD_C::_List_const_iterator<_Tp> __beyond = __last;
      ++__beyond;
      bool __whole = __first == __beyond;

Could you please make __whole const?

Thanks!