On 6/19/19 12:47 AM, Jonathan Wakely wrote:
On 18/06/19 22:42 +0200, François Dumont wrote:
On 6/18/19 12:54 PM, Jonathan Wakely wrote:
On 18/06/19 07:52 +0200, François Dumont wrote:
A small regression noticed while merging.
We shouldn't keep on using a moved-from key_type instance.
Ok to commit ? Feel free to do it if you prefer, I'll do so at end
of Europe day otherwise.
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h
b/libstdc++-v3/include/bits/hashtable_policy.h
index f5809c7443a..7e89e1b44c4 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -743,7 +743,8 @@ namespace __detail
std::tuple<>()
};
auto __pos
- = __h->_M_insert_unique_node(__k, __bkt, __code, __node._M_node);
+ =
__h->_M_insert_unique_node(__h->_M_extract()(__node._M_node->_M_v()),
+ __bkt, __code, __node._M_node);
__node._M_node = nullptr;
return __pos->second;
}
I can't create an example where this causes a problem, because the key
passed to _M_insert_unique_node is never used. So it doesn't matter
that it's been moved from.
So I have to wonder why we just added the key parameter to that
function, if it's never used.
I think you've been influence by my patch. I was using a
"_NodeAccessor" which wasn't giving access to the node without taking
owership so I needed to pass the key properly to compute new bucket
index in case of rehash.
But with your approach this change to the _M_insert_unique_node was
simply unecessary so here is a patch to cleanup this part.
Ha! I see, thanks. So I should have removed that key_type parameter
again after removing the NodeAccessor stuff.
Ok to commit ?
No, because that would restore the original signature of the
_M_insert_unique_node function, but it has changed contract. Old
callers who expect that function to delete the node would now leak
memory if an exception is thrown.
Oh, yes, abi, I tend to forget even if the recent PR 90920 remind me
about that, sorry.
If we change the contract of the function we need to change its
mangled name, so that callers expecting the old contract will not use
the new function.
I'll think about the best way to do that ...
Something like what's attached ?
I still use _GLIBCXX_INLINE_VERSION to tag functions that are kept just
for abi-compatibility.
Ok to commit after having run tests ?
François
diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index ab579a7059e..2ea75a24f1c 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -693,19 +693,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__node_base*
_M_get_previous_node(size_type __bkt, __node_base* __n);
- // Insert node __n with key __k and hash code __code, in bucket __bkt
- // if no rehash (assumes no element with same key already present).
+ // Insert node __n with hash code __code, in bucket __bkt if no
+ // rehash (assumes no element with same key already present).
// Takes ownership of __n if insertion succeeds, throws otherwise.
iterator
- _M_insert_unique_node(const key_type& __k, size_type __bkt,
- __hash_code __code, __node_type* __n,
- size_type __n_elt = 1);
+ _M_insert_node(true_type, size_type __bkt, __hash_code,
+ __node_type* __n, size_type __n_elt = 1);
+
+#if !_GLIBCXX_INLINE_VERSION
+ // Insert node with hash code __code, in bucket bkt if no rehash (assumes
+ // no element with its key already present). Take ownership of the node,
+ // deallocate it on exception.
+ iterator
+ _M_insert_unique_node(size_type __bkt, __hash_code __code,
+ __node_type* __n, size_type __n_elt = 1);
+#endif
// Insert node __n with key __k and hash code __code.
// Takes ownership of __n if insertion succeeds, throws otherwise.
iterator
- _M_insert_multi_node(__node_type* __hint, const key_type& __k,
+ _M_insert_node(false_type, __node_type* __hint,
+ __hash_code __code, __node_type* __n);
+
+#if !_GLIBCXX_INLINE_VERSION
+ // Insert node with hash code __code. Take ownership of the node,
+ // deallocate it on exception.
+ iterator
+ _M_insert_multi_node(__node_type* __hint,
__hash_code __code, __node_type* __n);
+#endif
template<typename... _Args>
std::pair<iterator, bool>
@@ -831,7 +847,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
else
{
__ret.position
- = _M_insert_unique_node(__k, __bkt, __code, __nh._M_ptr);
+ = _M_insert_node(true_type{}, __bkt, __code, __nh._M_ptr);
__nh._M_ptr = nullptr;
__ret.inserted = true;
}
@@ -851,7 +867,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
const key_type& __k = __nh._M_key();
auto __code = this->_M_hash_code(__k);
auto __ret
- = _M_insert_multi_node(__hint._M_cur, __k, __code, __nh._M_ptr);
+ = _M_insert_node(false_type{}, __hint._M_cur, __code, __nh._M_ptr);
__nh._M_ptr = nullptr;
return __ret;
}
@@ -918,8 +934,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
if (_M_find_node(__bkt, __k, __code) == nullptr)
{
auto __nh = __src.extract(__pos);
- _M_insert_unique_node(__k, __bkt, __code, __nh._M_ptr,
- __n_elt);
+ _M_insert_node(true_type{}, __bkt, __code, __nh._M_ptr, __n_elt);
__nh._M_ptr = nullptr;
__n_elt = 1;
}
@@ -1671,7 +1686,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return std::make_pair(iterator(__p), false);
// Insert the node
- auto __pos = _M_insert_unique_node(__k, __bkt, __code, __node._M_node);
+ auto __pos = _M_insert_node(true_type{}, __bkt, __code, __node._M_node);
__node._M_node = nullptr;
return { __pos, true };
}
@@ -1693,7 +1708,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__hash_code __code = this->_M_hash_code(__k);
auto __pos
- = _M_insert_multi_node(__hint._M_cur, __k, __code, __node._M_node);
+ = _M_insert_node(false_type{}, __hint._M_cur, __code, __node._M_node);
__node._M_node = nullptr;
return __pos;
}
@@ -1705,9 +1720,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
auto
_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
_H1, _H2, _Hash, _RehashPolicy, _Traits>::
- _M_insert_unique_node(const key_type& __k, size_type __bkt,
- __hash_code __code, __node_type* __node,
- size_type __n_elt)
+ _M_insert_node(true_type, size_type __bkt, __hash_code __code,
+ __node_type* __node, size_type __n_elt)
-> iterator
{
const __rehash_state& __saved_state = _M_rehash_policy._M_state();
@@ -1718,7 +1732,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
if (__do_rehash.first)
{
_M_rehash(__do_rehash.second, __saved_state);
- __bkt = _M_bucket_index(__k, __code);
+ __bkt = _M_bucket_index(this->_M_extract()(__node->_M_v()), __code);
}
this->_M_store_code(__node, __code);
@@ -1729,6 +1743,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return iterator(__node);
}
+#if !_GLIBCXX_INLINE_VERSION
template<typename _Key, typename _Value,
typename _Alloc, typename _ExtractKey, typename _Equal,
typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
@@ -1736,8 +1751,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
auto
_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
_H1, _H2, _Hash, _RehashPolicy, _Traits>::
- _M_insert_multi_node(__node_type* __hint, const key_type& __k,
- __hash_code __code, __node_type* __node)
+ _M_insert_unique_node(size_type __bkt, __hash_code __code,
+ __node_type* __node, size_type __n_elt)
+ -> iterator
+ {
+ __try
+ {
+ return _M_insert_node(true_type{}, __bkt, __code, __node, __n_elt);
+ }
+ __catch
+ {
+ this->_M_deallocate_node(__node);
+ __throw_exception_again;
+ }
+ }
+#endif
+
+ template<typename _Key, typename _Value,
+ typename _Alloc, typename _ExtractKey, typename _Equal,
+ typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
+ typename _Traits>
+ auto
+ _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
+ _H1, _H2, _Hash, _RehashPolicy, _Traits>::
+ _M_insert_node(false_type, __node_type* __hint,
+ __hash_code __code, __node_type* __node)
-> iterator
{
const __rehash_state& __saved_state = _M_rehash_policy._M_state();
@@ -1748,6 +1786,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_rehash(__do_rehash.second, __saved_state);
this->_M_store_code(__node, __code);
+ const key_type& __k = this->_M_extract()(__node->_M_v());
size_type __bkt = _M_bucket_index(__k, __code);
// Find the node before an equivalent one or use hint if it exists and
@@ -1782,6 +1821,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return iterator(__node);
}
+#if !_GLIBCXX_INLINE_VERSION
+ template<typename _Key, typename _Value,
+ typename _Alloc, typename _ExtractKey, typename _Equal,
+ typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
+ typename _Traits>
+ auto
+ _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
+ _H1, _H2, _Hash, _RehashPolicy, _Traits>::
+ _M_insert_multi_node(__node_type* __hint,
+ __hash_code __code, __node_type* __node)
+ -> iterator
+ {
+ __try
+ {
+ return _M_insert_node(false_type{}, __hint, __code, __node);
+ }
+ __catch
+ {
+ this->_M_deallocate_node(__node);
+ __throw_exception_again;
+ }
+ }
+#endif
+
// Insert v if no element with its key is already present.
template<typename _Key, typename _Value,
typename _Alloc, typename _ExtractKey, typename _Equal,
@@ -1804,7 +1867,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Scoped_node __node{ __node_gen(std::forward<_Arg>(__v)), this };
auto __pos
- = _M_insert_unique_node(__k, __bkt, __code, __node._M_node, __n_elt);
+ = _M_insert_node(true_type{}, __bkt, __code, __node._M_node, __n_elt);
__node._M_node = nullptr;
return { __pos, true };
}
@@ -1830,7 +1893,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Scoped_node __node{ __node_gen(std::forward<_Arg>(__v)), this };
const key_type& __k = this->_M_extract()(__node._M_node->_M_v());
auto __pos
- = _M_insert_multi_node(__hint._M_cur, __k, __code, __node._M_node);
+ = _M_insert_node(false_type{}, __hint._M_cur, __k, __code,
+ __node._M_node);
__node._M_node = nullptr;
return __pos;
}
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index f5809c7443a..6bb59e2be3a 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -716,7 +716,7 @@ namespace __detail
std::tuple<>()
};
auto __pos
- = __h->_M_insert_unique_node(__k, __bkt, __code, __node._M_node);
+ = __h->_M_insert_node(true_type{}, __bkt, __code, __node._M_node);
__node._M_node = nullptr;
return __pos->second;
}
@@ -743,7 +743,7 @@ namespace __detail
std::tuple<>()
};
auto __pos
- = __h->_M_insert_unique_node(__k, __bkt, __code, __node._M_node);
+ = __h->_M_insert_node(true_type{}, __bkt, __code, __node._M_node);
__node._M_node = nullptr;
return __pos->second;
}
