Default std::list default and move constructors

François Dumont frs.dumont@gmail.com
Wed Jul 12 20:12:00 GMT 2017


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.

>
> 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.

>
>> +    public:
>> +      _List_node_header() _GLIBCXX_NOEXCEPT
>> +      : _List_node_base(this, this)
>> +# if _GLIBCXX_USE_CXX11_ABI
>> +      , _M_size(0)
>> +# endif
>> +      { }
>
> This could be:
>
>      _List_node_header() _GLIBCXX_NOEXCEPT
>      { _M_init(); }

Sure, I was only interested in the initialization-list approach. As 
there is no default initialization of the members in _List_node_base for 
_M_prev/_M_next neither for _M_size I guess it will be super easy for 
the compiler to optimize it as if it was an initialization-list.

>
>
>> +#if __cplusplus >= 201103L
>> +      _List_node_header(_List_node_header&& __x) noexcept
>> +      : _List_node_base(__x._M_next, __x._M_prev)
>
> And this could use aggregate-initialization:
>
>      : _List_node_base{__x._M_next, __x._M_prev}

Nice.

>> +#if _GLIBCXX_USE_CXX11_ABI
>> +      size_t _M_get_size() const { return _M_size; }
>> +      void _M_set_size(size_t __n) { _M_size = __n; }
>> +      void _M_inc_size(size_t __n) { _M_size += __n; }
>> +      void _M_dec_size(size_t __n) { _M_size -= __n; }
>> +#else
>> +      // dummy implementations used when the size is not stored
>> +      size_t _M_get_size() const { return 0; }
>> +      void _M_set_size(size_t) { }
>> +      void _M_inc_size(size_t) { }
>> +      void _M_dec_size(size_t) { }
>> +#endif
>
> What do you think about only having _M_set_size() here?
> The other functions are not needed here (assuming _M_size is public).
>
> We could even get rid of _M_set_size and use #if in _M_init:
>
>> +      void
>> +      _M_init() _GLIBCXX_NOEXCEPT
>> +      {
>> +    this->_M_next = this->_M_prev = this;
> #if _GLIBCXX_USE_CXX11_ABI
>        _M_size = 0;
> #endif
>
>> +    _M_set_size(0);
>> +      }
>> +    };
>
> This replaces a #if and #else and four functions with a #if and no
> functions, which seems simpler to me.
> _List_impl _M_impl;
>>
>> -#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.

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.

Tests running, ok to commit if successful ?

François

-------------- next part --------------
A non-text attachment was scrubbed...
Name: list.patch
Type: text/x-patch
Size: 10314 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20170712/00ecb753/attachment.bin>


More information about the Libstdc++ mailing list