Re: Default std::list default and move constructors

[Jonathan Wakely <jwakely@redhat.com>]

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!