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.
Ok to commit ?
As far as I can tell, it would only be used for a non-default range
hash function, and I don't care about that. Frankly I find the
policy-based _Hashtable completely unmaintainable and I'd gladly get
rid of all of it that isn't needed for the std::unordered_xxx
containers. The non-standard extensions are not used by anybody,
apparently not tested properly (or this regression should have been
noticed) and make the code too complicated.
I never consider removing this but it would indeed make maintainance
easy. I'll add to my TODO.
We're adding new parameters that have to be passed around even though
they're never used by 99.999% of users. No wonder the code is only
fast at -O3.
</rant>
diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index ab579a7059e..41edafaa2e3 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -693,11 +693,11 @@ _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,
+ _M_insert_unique_node(size_type __bkt,
__hash_code __code, __node_type* __n,
size_type __n_elt = 1);
@@ -831,7 +831,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
else
{
__ret.position
- = _M_insert_unique_node(__k, __bkt, __code, __nh._M_ptr);
+ = _M_insert_unique_node(__bkt, __code, __nh._M_ptr);
__nh._M_ptr = nullptr;
__ret.inserted = true;
}
@@ -918,8 +918,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_unique_node(__bkt, __code, __nh._M_ptr, __n_elt);
__nh._M_ptr = nullptr;
__n_elt = 1;
}
@@ -1671,7 +1670,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_unique_node(__bkt, __code, __node._M_node);
__node._M_node = nullptr;
return { __pos, true };
}
@@ -1705,9 +1704,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_unique_node(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 +1716,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);
@@ -1804,7 +1802,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_unique_node(__bkt, __code, __node._M_node, __n_elt);
__node._M_node = nullptr;
return { __pos, true };
}
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index f5809c7443a..6646b9f84a8 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_unique_node(__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_unique_node(__bkt, __code, __node._M_node);
__node._M_node = nullptr;
return __pos->second;
}
