[PATCH][Hashtable 3/6] Fix noexcept qualifications
François Dumont
frs.dumont@gmail.com
Wed Jul 29 09:33:24 GMT 2020
I eventually committed the attached patch which consider all your
remarks and moreover put back the move constructor implementation where
it used to be (not inline). It limits the size of the patch.
I also added comments on true_type/false_type new usages like you
advised on [Hashtable 0/6] thread.
François
On 17/07/20 11:55 am, Jonathan Wakely wrote:
> On 17/11/19 21:59 +0100, François Dumont wrote:
>> This patch adds noexcept qualification on allocator aware
>> constructors and fix the one on the default constructor.
>>
>> Â Â Â * include/bits/hashtable.h
>> Â Â Â (_Hashtable(_Hashtable&& __ht, __node_alloc_type&& __a,
>> true_type)):
>> Â Â Â Add noexcept qualification.
>> Â Â Â (_Hashtable(_Hashtable&&)): Fix noexcept qualification.
>> Â Â Â (_Hashtable(_Hashtable&&, const allocator_type&)): Add noexcept
>> Â Â Â qualification.
>> Â Â Â * include/bits/unordered_map.h
>> Â Â Â (unordered_map(unordered_map&&, const allocator_type&)): Add
>> noexcept
>> Â Â Â qualification.
>> Â Â Â (unordered_multimap(unordered_multimap&&, const
>> allocator_type&)): Add
>> Â Â Â noexcept qualification.
>> Â Â Â * include/bits/unordered_set.h
>> Â Â Â (unordered_set(unordered_set&&, const allocator_type&)): Add
>> noexcept
>> Â Â Â qualification.
>> Â Â Â (unordered_multiset(unordered_multiset&&, const
>> allocator_type&)): Add
>> Â Â Â noexcept qualification.
>> Â Â Â *
>> testsuite/23_containers/unordered_map/allocator/default_init.cc:
>> Â Â Â New.
>> Â Â Â * testsuite/23_containers/unordered_map/cons/
>> Â Â Â noexcept_default_construct.cc: New.
>> Â Â Â * testsuite/23_containers/unordered_map/cons/
>> Â Â Â noexcept_move_construct.cc: New.
>> Â Â Â * testsuite/23_containers/unordered_map/modifiers/move_assign.cc:
>> Â Â Â New.
>> Â Â Â * testsuite/23_containers/unordered_multimap/cons/
>> Â Â Â noexcept_default_construct.cc: New.
>> Â Â Â * testsuite/23_containers/unordered_multimap/cons/
>> Â Â Â noexcept_move_construct.cc: New.
>> Â Â Â * testsuite/23_containers/unordered_multiset/cons/
>> Â Â Â noexcept_default_construct.cc: New.
>> Â Â Â * testsuite/23_containers/unordered_multiset/cons/
>> Â Â Â noexcept_move_construct.cc: New.
>> Â Â Â *
>> testsuite/23_containers/unordered_set/allocator/default_init.cc: New.
>> Â Â Â * testsuite/23_containers/unordered_set/cons/
>> Â Â Â noexcept_default_construct.cc: New.
>> Â Â Â *
>> testsuite/23_containers/unordered_set/cons/noexcept_move_construct.cc:
>> Â Â Â New.
>>
>> Tested under Linux x86_64.
>>
>> François
>>
>
>> diff --git a/libstdc++-v3/include/bits/hashtable.h
>> b/libstdc++-v3/include/bits/hashtable.h
>> index 5f785d4904d..ad07a36eb83 100644
>> --- a/libstdc++-v3/include/bits/hashtable.h
>> +++ b/libstdc++-v3/include/bits/hashtable.h
>> @@ -463,6 +463,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> __hashtable_alloc(__node_alloc_type(__a))
>> { }
>>
>> + _Hashtable(_Hashtable&& __ht, __node_alloc_type&& __a, true_type)
>> + noexcept(std::is_nothrow_copy_constructible<_H1>::value &&
>> + std::is_nothrow_copy_constructible<_Equal>::value)
>> + : __hashtable_base(__ht),
>> + __map_base(__ht),
>> + __rehash_base(__ht),
>> + __hashtable_alloc(std::move(__a)),
>> + _M_buckets(__ht._M_buckets),
>> + _M_bucket_count(__ht._M_bucket_count),
>> + _M_before_begin(__ht._M_before_begin._M_nxt),
>> + _M_element_count(__ht._M_element_count),
>> + _M_rehash_policy(__ht._M_rehash_policy)
>> + {
>> + // Update, if necessary, buckets if __ht is using its single
>> bucket.
>
> I know the comment was already present, but I can't parse this.
>
> There are two 'if' conditions, but don't they refer to the same thing?
> It's necessary if __ht is using a single bucket, so shouldn't it be
> either "Update buckets if __ht is using its single bucket" or "Update
> buckets if necessary"?
>
> "Update buckets if __ht is using its single bucket" seems preferable.
>
>> + if (__ht._M_uses_single_bucket())
>> + {
>> + _M_buckets = &_M_single_bucket;
>> + _M_single_bucket = __ht._M_single_bucket;
>> + }
>> +
>> + // Fix bucket containing the _M_before_begin pointer that can't be
>> + // moved.
>> + _M_update_bbegin();
>> +
>> + __ht._M_reset();
>> + }
>> +
>> + _Hashtable(_Hashtable&&, __node_alloc_type&&, false_type);
>> +
>> template<typename _InputIterator>
>> _Hashtable(_InputIterator __first, _InputIterator __last,
>> size_type __bkt_count_hint,
>> @@ -489,11 +518,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>
>> _Hashtable(const _Hashtable&);
>>
>> - _Hashtable(_Hashtable&&) noexcept;
>> + _Hashtable(_Hashtable&& __ht)
>> + noexcept( noexcept(
>> + _Hashtable(std::declval<_Hashtable&&>(),
>
> The && here is redundant, declval adds && to the type anyway.
>
>> + std::declval<__node_alloc_type&&>(), std::declval<true_type>())) )
>
> Same for __node_alloc_type&& here.
>
> And true_type{} is shorter and simpler than std::declval<true_type>().
> We know is_nothrow_default_constructible<true_type> is true, so we can
> just use that.
>
>> + : _Hashtable(std::move(__ht),
>> std::move(__ht._M_node_allocator()),
>> + true_type{})
>> + { }
>>
>> _Hashtable(const _Hashtable&, const allocator_type&);
>>
>> - _Hashtable(_Hashtable&&, const allocator_type&);
>> + _Hashtable(_Hashtable&& __ht, const allocator_type& __a)
>> + noexcept( noexcept(
>> + _Hashtable(std::declval<_Hashtable&&>(),
>> + std::declval<__node_alloc_type&&>(),
>
> Redundant &&s.
>
>> + std::declval<typename
>> __node_alloc_traits::is_always_equal>())) )
>
> Redundant declval: typename __node_alloc_traits::is_always_equal{}
>
>> + : _Hashtable(std::move(__ht), __node_alloc_type(__a),
>> + typename __node_alloc_traits::is_always_equal{})
>> + { }
>>
>> // Use delegating constructors.
>> template<typename _InputIterator>
>> @@ -1368,36 +1410,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> _M_assign(__ht, __alloc_node_gen);
>> }
>>
>> - template<typename _Key, typename _Value,
>> - typename _Alloc, typename _ExtractKey, typename _Equal,
>> - typename _H1, typename _H2, typename _Hash, typename
>> _RehashPolicy,
>> - typename _Traits>
>> - _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
>> - _H1, _H2, _Hash, _RehashPolicy, _Traits>::
>> - _Hashtable(_Hashtable&& __ht) noexcept
>> - : __hashtable_base(__ht),
>> - __map_base(__ht),
>> - __rehash_base(__ht),
>> - __hashtable_alloc(std::move(__ht._M_base_alloc())),
>> - _M_buckets(__ht._M_buckets),
>> - _M_bucket_count(__ht._M_bucket_count),
>> - _M_before_begin(__ht._M_before_begin._M_nxt),
>> - _M_element_count(__ht._M_element_count),
>> - _M_rehash_policy(__ht._M_rehash_policy)
>> - {
>> - // Update, if necessary, buckets if __ht is using its single
>> bucket.
>> - if (__ht._M_uses_single_bucket())
>> - {
>> - _M_buckets = &_M_single_bucket;
>> - _M_single_bucket = __ht._M_single_bucket;
>> - }
>> -
>> - // Fix bucket containing the _M_before_begin pointer that
>> can't be moved.
>> - _M_update_bbegin();
>> -
>> - __ht._M_reset();
>> - }
>> -
>> template<typename _Key, typename _Value,
>> typename _Alloc, typename _ExtractKey, typename _Equal,
>> typename _H1, typename _H2, typename _Hash, typename
>> _RehashPolicy,
>> @@ -1424,11 +1436,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> typename _Traits>
>> _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
>> _H1, _H2, _Hash, _RehashPolicy, _Traits>::
>> - _Hashtable(_Hashtable&& __ht, const allocator_type& __a)
>> + _Hashtable(_Hashtable&& __ht, __node_alloc_type&& __a, false_type)
>> : __hashtable_base(__ht),
>> __map_base(__ht),
>> __rehash_base(__ht),
>> - __hashtable_alloc(__node_alloc_type(__a)),
>> + __hashtable_alloc(std::move(__a)),
>> _M_buckets(nullptr),
>> _M_bucket_count(__ht._M_bucket_count),
>> _M_element_count(__ht._M_element_count),
>> diff --git a/libstdc++-v3/include/bits/unordered_map.h
>> b/libstdc++-v3/include/bits/unordered_map.h
>> index 49c175a90be..5131e02e8aa 100644
>> --- a/libstdc++-v3/include/bits/unordered_map.h
>> +++ b/libstdc++-v3/include/bits/unordered_map.h
>> @@ -209,6 +209,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> */
>> unordered_map(unordered_map&& __umap,
>> const allocator_type& __a)
>> + noexcept( noexcept(_Hashtable(declval<_Hashtable&&>(), __a)) )
>
> declval should be qualified. It might be simpler to just say:
>
> noexcept( noexcept(_Hashtable(std::move(__umap._M_h), __a)) )
>
>> : _M_h(std::move(__umap._M_h), __a)
>> { }
>>
>> @@ -1356,6 +1357,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> */
>> unordered_multimap(unordered_multimap&& __ummap,
>> const allocator_type& __a)
>> + noexcept( noexcept(_Hashtable(declval<_Hashtable&&>(), __a)) )
>
> Same here.
>
>> : _M_h(std::move(__ummap._M_h), __a)
>> { }
>>
>> diff --git a/libstdc++-v3/include/bits/unordered_set.h
>> b/libstdc++-v3/include/bits/unordered_set.h
>> index 855579ee623..7154ec843db 100644
>> --- a/libstdc++-v3/include/bits/unordered_set.h
>> +++ b/libstdc++-v3/include/bits/unordered_set.h
>> @@ -203,6 +203,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> */
>> unordered_set(unordered_set&& __uset,
>> const allocator_type& __a)
>> + noexcept( noexcept(_Hashtable(declval<_Hashtable&&>(), __a)) )
>
> Same here.
>
>> : _M_h(std::move(__uset._M_h), __a)
>> { }
>>
>> @@ -1044,6 +1045,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>> */
>> unordered_multiset(unordered_multiset&& __umset,
>> const allocator_type& __a)
>> + noexcept( noexcept(_Hashtable(declval<_Hashtable&&>(), __a)) )
>
> Same here.
>
>> : _M_h(std::move(__umset._M_h), __a)
>> { }
>>
>> diff --git
>> a/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/default_init.cc
>> b/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/default_init.cc
>>
>> new file mode 100644
>> index 00000000000..8c3103c2614
>> --- /dev/null
>> +++
>> b/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/default_init.cc
>> @@ -0,0 +1,69 @@
>> +// Copyright (C) 2019 Free Software Foundation, Inc.
>
> Please remember to update the dates in the new tests.
>
> OK for master with those adjustments. Thanks.
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hashtable.patch
Type: text/x-patch
Size: 35948 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20200729/b3eafde4/attachment-0001.bin>
More information about the Libstdc++
mailing list