| Message ID | 4d30deb3-8720-2aa6-d0f2-94f6fca44825@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 06/10/16 22:17 +0200, François Dumont wrote: >Another approach is to rely on existing compiler ability to compute >conditional noexcept when defaulting implementations. This is what I >have done in this patch. > >The new default constructor on _Rb_tree_node_base is not a problem as >it is not used to build _Rb_tree_node. Why not? >I'll try to do the same for copy constructor/assignment and move >constructor/assignment. We need to make sure we don't change whether any of those operations are trivial (which shouldn't be a problem for copy/move, because they are definitely very non-trivial and will stay that way!) Does this change the default constructors from non-trivial to trivial? >--- a/libstdc++-v3/include/bits/stl_tree.h >+++ b/libstdc++-v3/include/bits/stl_tree.h >@@ -108,6 +108,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _Base_ptr _M_left; > _Base_ptr _M_right; > >+ _Rb_tree_node_base() _GLIBCXX_NOEXCEPT >+ : _M_color(_S_red), _M_parent(0), _M_left(this), _M_right(this) >+ { } >+ > static _Base_ptr > _S_minimum(_Base_ptr __x) _GLIBCXX_NOEXCEPT > { Another option would be: struct _Head_node : _Rb_tree_node_base { _Head_node() { _M_color = _S_red; _M_parent = _Base_ptr(); _M_left = _M_right = this; } }; >@@ -603,23 +607,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > { > _Key_compare _M_key_compare; > _Rb_tree_node_base _M_header; _Head_node _M_header; That way *only* this node gets the zero-initialization, not all node bases. With either solution we can get rid of _M_header() in every ctor-initializer-list. >+#if __cplusplus < 201103L > size_type _M_node_count; // Keeps track of size of tree. >+#else >+ size_type _M_node_count = 0; // Keeps track of size of tree. >+#endif > >+#if __cplusplus < 201103L > _Rb_tree_impl() > : _Node_allocator(), _M_key_compare(), _M_header(), > _M_node_count(0) >- { _M_initialize(); } >+ { } >+#else >+ _Rb_tree_impl() = default; >+#endif > > _Rb_tree_impl(const _Key_compare& __comp, const _Node_allocator& __a) >- : _Node_allocator(__a), _M_key_compare(__comp), _M_header(), >- _M_node_count(0) >- { _M_initialize(); } >+ : _Node_allocator(__a), _M_key_compare(__comp), _M_header() >+#if __cplusplus < 201103L >+ , _M_node_count(0) >+#endif Doing this conditionally seems pointless, why not just set it here unconditionally?
On 06/10/2016 23:34, Jonathan Wakely wrote: > On 06/10/16 22:17 +0200, François Dumont wrote: >> Another approach is to rely on existing compiler ability to compute >> conditional noexcept when defaulting implementations. This is what I >> have done in this patch. >> >> The new default constructor on _Rb_tree_node_base is not a problem as >> it is not used to build _Rb_tree_node. > > Why not? _Rb_tree_node_base is used in 2 context. As member of _Rb_tree_impl in which case we need the new default constructor. And also as base class of _Rb_tree_node which is never constructed. Nodes are being allocated and then associated value is being constructed through the allocator, the node default constructor itself is never invoked. If you think it is cleaner to create an intermediate type that will take care of this initialization through its default constructor I can do that. > >> I'll try to do the same for copy constructor/assignment and move >> constructor/assignment. > > We need to make sure we don't change whether any of those operations > are trivial (which shouldn't be a problem for copy/move, because they > are definitely very non-trivial and will stay that way!) > > Does this change the default constructors from non-trivial to trivial? It would be a major compiler bug if making a constructor default was making it trivial. > > >> --- a/libstdc++-v3/include/bits/stl_tree.h >> +++ b/libstdc++-v3/include/bits/stl_tree.h >> @@ -108,6 +108,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> _Base_ptr _M_left; >> _Base_ptr _M_right; >> >> + _Rb_tree_node_base() _GLIBCXX_NOEXCEPT >> + : _M_color(_S_red), _M_parent(0), _M_left(this), _M_right(this) >> + { } >> + >> static _Base_ptr >> _S_minimum(_Base_ptr __x) _GLIBCXX_NOEXCEPT >> { > > Another option would be: > > struct _Head_node : _Rb_tree_node_base { > _Head_node() { > _M_color = _S_red; > _M_parent = _Base_ptr(); > _M_left = _M_right = this; > } > }; If you want something like that I would rather do: _Rb_tree_node_base() = default; _Rb_tree_node_base(int) _GLIBCXX_NO_EXCEPT : _M_color(_S_red), _M_parent(0), _M_left(this), _M_right(this) { } and then: struct _Head_node : _Rb_tree_node_base { _Head_node() _GLIBCXX_NO_EXCEPT : _Rb_tree_node_base(0) { } }; but as I already said the default constructor on _Rb_tree_node_base works just fine. > >> @@ -603,23 +607,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> { >> _Key_compare _M_key_compare; >> _Rb_tree_node_base _M_header; > > _Head_node _M_header; > > That way *only* this node gets the zero-initialization, not all node > bases. > > With either solution we can get rid of _M_header() in every > ctor-initializer-list. I am already preparing another patch, I will add this simplification. > > >> +#if __cplusplus < 201103L >> size_type _M_node_count; // Keeps track of size of tree. >> +#else >> + size_type _M_node_count = 0; // Keeps track of size of >> tree. >> +#endif >> >> +#if __cplusplus < 201103L >> _Rb_tree_impl() >> : _Node_allocator(), _M_key_compare(), _M_header(), >> _M_node_count(0) >> - { _M_initialize(); } >> + { } >> +#else >> + _Rb_tree_impl() = default; >> +#endif >> >> _Rb_tree_impl(const _Key_compare& __comp, const >> _Node_allocator& __a) >> - : _Node_allocator(__a), _M_key_compare(__comp), _M_header(), >> - _M_node_count(0) >> - { _M_initialize(); } >> + : _Node_allocator(__a), _M_key_compare(__comp), _M_header() >> +#if __cplusplus < 201103L >> + , _M_node_count(0) >> +#endif > > Doing this conditionally seems pointless, why not just set it here > unconditionally I wasn't sure about this one, just seemed clearner to not do this 0 initialization again as already done at class definition level. If compiler just moved it away then ok, I can remove the check. François
On 08/10/16 22:55 +0200, François Dumont wrote: >On 06/10/2016 23:34, Jonathan Wakely wrote: >>On 06/10/16 22:17 +0200, François Dumont wrote: >>>Another approach is to rely on existing compiler ability to >>>compute conditional noexcept when defaulting implementations. This >>>is what I have done in this patch. >>> >>>The new default constructor on _Rb_tree_node_base is not a problem >>>as it is not used to build _Rb_tree_node. >> >>Why not? > >_Rb_tree_node_base is used in 2 context. As member of _Rb_tree_impl in >which case we need the new default constructor. And also as base class >of _Rb_tree_node which is never constructed. Nodes are being allocated >and then associated value is being constructed through the allocator, >the node default constructor itself is never invoked. In C++03 mode that is true, but it's only valid because the type is trivially-constructible. If the type requires "non-vacuous initialization" then it's not valid to allocate memory for it and start using it without invoking a constructor. If you add a non-trivial constructor then we can't do that any more. In C++11 and later, see line 550 in <bits/stl_tree.h> ::new(__node) _Rb_tree_node<_Val>; This default-constructs a tree node. Currently there is no user-provided default constructor, so default-construction does no initialization. Adding your constructor would mean it is used for every node. > If you think it is cleaner to create an intermediate type that >will take care of this initialization through its default constructor >I can do that. > >> >>>I'll try to do the same for copy constructor/assignment and move >>>constructor/assignment. >> >>We need to make sure we don't change whether any of those operations >>are trivial (which shouldn't be a problem for copy/move, because they >>are definitely very non-trivial and will stay that way!) >> >>Does this change the default constructors from non-trivial to trivial? >It would be a major compiler bug if making a constructor default was >making it trivial. I must be misunderstanding you, because this is not a bug: #include <type_traits> struct A { A() { } }; static_assert( !std::is_trivially_default_constructible<A>::value, "" ); struct B { B() = default; }; static_assert( std::is_trivially_default_constructible<B>::value, "" );
On 09/10/16 16:14 +0100, Jonathan Wakely wrote: >On 08/10/16 22:55 +0200, François Dumont wrote: >>On 06/10/2016 23:34, Jonathan Wakely wrote: >>>On 06/10/16 22:17 +0200, François Dumont wrote: >>>>Another approach is to rely on existing compiler ability to >>>>compute conditional noexcept when defaulting implementations. >>>>This is what I have done in this patch. >>>> >>>>The new default constructor on _Rb_tree_node_base is not a >>>>problem as it is not used to build _Rb_tree_node. >>> >>>Why not? >> >>_Rb_tree_node_base is used in 2 context. As member of _Rb_tree_impl >>in which case we need the new default constructor. And also as base >>class of _Rb_tree_node which is never constructed. Nodes are being >>allocated and then associated value is being constructed through the >>allocator, the node default constructor itself is never invoked. > >In C++03 mode that is true, but it's only valid because the type is >trivially-constructible. If the type requires "non-vacuous >initialization" then it's not valid to allocate memory for it and >start using it without invoking a constructor. If you add a >non-trivial constructor then we can't do that any more. In fact that code is highly questionable, because the element member makes the node require non-trivial initialization. We rely on the base being trivially-constructible, and the element being constructed by the allocator, and assume that it's OK to then treat that memory as a node. In fact only its base class and member have been constructed, not the node itself. The C++11 version (using an aligned buffer) is correct. But I'd prefer not to make the C++98 version worse.
diff --git a/libstdc++-v3/include/bits/stl_map.h b/libstdc++-v3/include/bits/stl_map.h index e5b2a1b..dea7d5b 100644 --- a/libstdc++-v3/include/bits/stl_map.h +++ b/libstdc++-v3/include/bits/stl_map.h @@ -167,11 +167,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER /** * @brief Default constructor creates no elements. */ - map() - _GLIBCXX_NOEXCEPT_IF( - is_nothrow_default_constructible<allocator_type>::value - && is_nothrow_default_constructible<key_compare>::value) - : _M_t() { } +#if __cplusplus < 201103L + map() : _M_t() { } +#else + map() = default; +#endif /** * @brief Creates a %map with no elements. diff --git a/libstdc++-v3/include/bits/stl_multimap.h b/libstdc++-v3/include/bits/stl_multimap.h index d240427..7e86b76 100644 --- a/libstdc++-v3/include/bits/stl_multimap.h +++ b/libstdc++-v3/include/bits/stl_multimap.h @@ -164,11 +164,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER /** * @brief Default constructor creates no elements. */ - multimap() - _GLIBCXX_NOEXCEPT_IF( - is_nothrow_default_constructible<allocator_type>::value - && is_nothrow_default_constructible<key_compare>::value) - : _M_t() { } +#if __cplusplus < 201103L + multimap() : _M_t() { } +#else + multimap() = default; +#endif /** * @brief Creates a %multimap with no elements. diff --git a/libstdc++-v3/include/bits/stl_multiset.h b/libstdc++-v3/include/bits/stl_multiset.h index cc068a9..7fe2fbd 100644 --- a/libstdc++-v3/include/bits/stl_multiset.h +++ b/libstdc++-v3/include/bits/stl_multiset.h @@ -144,11 +144,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER /** * @brief Default constructor creates no elements. */ - multiset() - _GLIBCXX_NOEXCEPT_IF( - is_nothrow_default_constructible<allocator_type>::value - && is_nothrow_default_constructible<key_compare>::value) - : _M_t() { } +#if __cplusplus < 201103L + multiset() : _M_t() { } +#else + multiset() = default; +#endif /** * @brief Creates a %multiset with no elements. diff --git a/libstdc++-v3/include/bits/stl_set.h b/libstdc++-v3/include/bits/stl_set.h index 3938351..5ed9672 100644 --- a/libstdc++-v3/include/bits/stl_set.h +++ b/libstdc++-v3/include/bits/stl_set.h @@ -147,11 +147,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER /** * @brief Default constructor creates no elements. */ - set() - _GLIBCXX_NOEXCEPT_IF( - is_nothrow_default_constructible<allocator_type>::value - && is_nothrow_default_constructible<key_compare>::value) - : _M_t() { } +#if __cplusplus < 201103L + set() : _M_t() { } +#else + set() = default; +#endif /** * @brief Creates a %set with no elements. diff --git a/libstdc++-v3/include/bits/stl_tree.h b/libstdc++-v3/include/bits/stl_tree.h index ee2dc70..ed575d0 100644 --- a/libstdc++-v3/include/bits/stl_tree.h +++ b/libstdc++-v3/include/bits/stl_tree.h @@ -108,6 +108,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Base_ptr _M_left; _Base_ptr _M_right; + _Rb_tree_node_base() _GLIBCXX_NOEXCEPT + : _M_color(_S_red), _M_parent(0), _M_left(this), _M_right(this) + { } + static _Base_ptr _S_minimum(_Base_ptr __x) _GLIBCXX_NOEXCEPT { @@ -603,23 +607,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { _Key_compare _M_key_compare; _Rb_tree_node_base _M_header; +#if __cplusplus < 201103L size_type _M_node_count; // Keeps track of size of tree. +#else + size_type _M_node_count = 0; // Keeps track of size of tree. +#endif +#if __cplusplus < 201103L _Rb_tree_impl() : _Node_allocator(), _M_key_compare(), _M_header(), _M_node_count(0) - { _M_initialize(); } + { } +#else + _Rb_tree_impl() = default; +#endif _Rb_tree_impl(const _Key_compare& __comp, const _Node_allocator& __a) - : _Node_allocator(__a), _M_key_compare(__comp), _M_header(), - _M_node_count(0) - { _M_initialize(); } + : _Node_allocator(__a), _M_key_compare(__comp), _M_header() +#if __cplusplus < 201103L + , _M_node_count(0) +#endif + { } #if __cplusplus >= 201103L _Rb_tree_impl(const _Key_compare& __comp, _Node_allocator&& __a) - : _Node_allocator(std::move(__a)), _M_key_compare(__comp), - _M_header(), _M_node_count(0) - { _M_initialize(); } + : _Node_allocator(std::move(__a)), _M_key_compare(__comp), + _M_header() + { } #endif void @@ -630,16 +644,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION this->_M_header._M_right = &this->_M_header; this->_M_node_count = 0; } - - private: - void - _M_initialize() - { - this->_M_header._M_color = _S_red; - this->_M_header._M_parent = 0; - this->_M_header._M_left = &this->_M_header; - this->_M_header._M_right = &this->_M_header; - } }; _Rb_tree_impl<_Compare> _M_impl; @@ -831,7 +835,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION public: // allocation/deallocation +#if __cplusplus < 201103L _Rb_tree() { } +#else + _Rb_tree() = default; +#endif _Rb_tree(const _Compare& __comp, const allocator_type& __a = allocator_type())