diff mbox

77864 Fix noexcept conditions for map/set default constructors

Message ID 400a3910-c407-1071-5721-4e410d293c21@gmail.com
State New
Headers show

Commit Message

François Dumont Oct. 25, 2016, 7:55 p.m. UTC 
On 24/10/2016 13:03, Jonathan Wakely wrote:
> 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

Indeed, here is the simplified patch.

But I will come back to this base type for the next patch to generalize 
the defaulted constructors and assignment operators.

Tested under Linux x86_64, ok to commit ?

François

Comments

Jonathan Wakely Oct. 26, 2016, 8:21 a.m. UTC | #1 
On 25/10/16 21:55 +0200, François Dumont wrote:
>Indeed, here is the simplified patch.

>

>But I will come back to this base type for the next patch to 

>generalize the defaulted constructors and assignment operators.

>

>Tested under Linux x86_64, ok to commit ?


OK for trunk, thanks.
diff mbox

Patch

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..0bc5f4b 100644
--- a/libstdc++-v3/include/bits/stl_tree.h
+++ b/libstdc++-v3/include/bits/stl_tree.h
@@ -606,6 +606,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  size_type		_M_node_count; // Keeps track of size of tree.
 
 	  _Rb_tree_impl()
+	  _GLIBCXX_NOEXCEPT_IF(
+	    is_nothrow_default_constructible<_Node_allocator>::value
+	    && is_nothrow_default_constructible<_Key_compare>::value)
 	  : _Node_allocator(), _M_key_compare(), _M_header(),
 	    _M_node_count(0)
 	  { _M_initialize(); }
@@ -831,7 +834,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())