[PATCH] 77864 Fix noexcept conditions for map/set default constructors
Jonathan Wakely
jwakely@redhat.com
Mon Oct 24 11:03:00 GMT 2016
On 12/10/16 22:36 +0200, François Dumont wrote:
>On 10/10/2016 23:01, Tim Song wrote:
>>Trying again...with a few edits.
>>
>>>On Mon, Oct 10, 2016 at 3:24 PM, François Dumont <frs.dumont@gmail.com>
>>>wrote:
>>>
>>>@@ -602,24 +612,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>> struct _Rb_tree_impl : public _Node_allocator
>>> {
>>> _Key_compare _M_key_compare;
>>>- _Rb_tree_node_base _M_header;
>>>+ _Rb_header_node _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(); }
>>>+ : _M_node_count(0)
>>>+ { }
>>>+#else
>>>+ _Rb_tree_impl() = default;
>>>+#endif
>>
>>The default constructor of the associative containers is required to
>>value-initialize the comparator (see their synopses in
>>[map/set/multimap/multiset.overview]).
>I don't have latest Standard version so can't see the exact word but I
>find quite annoying that the Standard doesn't allow this simple
>implementation.
>
>I don't know if unodered containers have same kind of requirements for
>equal or hash functors but if so current implementation doesn't do
>this value initialization.
>
>So here is another attempt. This time it simply allows to have
>noexcept condition in one place and closer to where operations are
>being invoked.
>
>Ok to commit after tests ?
>
>François
>>
>> _Rb_tree_impl() = default; doesn't do that; it default-initializes the
>> comparator instead.
>>
>>Tim
>>
>
>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..b6a3c1e 100644
>--- a/libstdc++-v3/include/bits/stl_tree.h
>+++ b/libstdc++-v3/include/bits/stl_tree.h
>@@ -137,6 +137,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> }
> };
>
>+ struct _Rb_header_node : public _Rb_tree_node_base
>+ {
>+ _Rb_header_node() _GLIBCXX_NOEXCEPT
>+ {
>+ _M_color = _S_red;
>+ _M_parent = _Base_ptr();
>+ _M_left = _M_right = this;
>+ }
>+ };
>+
> template<typename _Val>
> struct _Rb_tree_node : public _Rb_tree_node_base
> {
>@@ -602,24 +612,34 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> struct _Rb_tree_impl : public _Node_allocator
> {
> _Key_compare _M_key_compare;
>- _Rb_tree_node_base _M_header;
>+ _Rb_header_node _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
>
> _Rb_tree_impl()
>- : _Node_allocator(), _M_key_compare(), _M_header(),
>- _M_node_count(0)
>- { _M_initialize(); }
>+ _GLIBCXX_NOEXCEPT_IF(
>+ is_nothrow_default_constructible<_Node_allocator>::value
>+ && is_nothrow_default_constructible<_Key_compare>::value)
>+ : _M_key_compare()
>+#if __cplusplus < 201103L
>+ , _M_node_count(0)
>+#endif
>+ { }
I still think this part is pointless. Why use conditional compilation
here, when we could just always set it to zero?
What is the advantage of using #if here, except adding more lines of
code?
>
> _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)
>+#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)
>+ { }
> #endif
>
> void
>@@ -630,16 +650,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;
>- }
> };
Since we can't remove the constructors that called _M_initialize() we
don't need to get rid of _M_initialize() either. That means we don't
need the _Rb_header_node type (so don't need to produce RTTI for a new
type).
The purpose of the patch is to allow _Rb_tree() = default and then
set() = default, map() = default etc.
That can be done by simply putting the _GLIBCXX_NOEXCEPT_IF on
_Rb_tree_impl() and forgetting all the other changes to _Rb_tree_impl.
Using a default member initializer for _M_node_count and removing
_M_initialize() aren't necessary to achieve the purpose of the patch.
More information about the Libstdc++
mailing list