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