On 24/05/22 12:18, Jonathan Wakely wrote:
On Thu, 5 May 2022 at 18:38, François Dumont via Libstdc++
<libstd...@gcc.gnu.org> wrote:
Hi
Renewing my patch to fix PR 56112 but for the insert methods, I totally
change it, now works also with move-only key types.
I let you Jonathan find a better name than _ValueTypeEnforcer as usual :-)
libstdc++: [_Hashtable] Insert range of types convertible to value_type
PR 56112
Fix insertion of range of types convertible to value_type. Fix also when
this value_type
has a move-only key_type which also allow converted values to be moved.
libstdc++-v3/ChangeLog:
PR libstdc++/56112
* include/bits/hashtable_policy.h (_ValueTypeEnforcer): New.
* include/bits/hashtable.h
(_Hashtable<>::_M_insert_unique_aux): New.
(_Hashtable<>::_M_insert(_Arg&&, const _NodeGenerator&,
true_type)): Use latters.
(_Hashtable<>::_M_insert(_Arg&&, const _NodeGenerator&,
false_type)): Likewise.
(_Hashtable(_InputIterator, _InputIterator, size_type, const
_Hash&, const _Equal&,
const allocator_type&, true_type)): Use this.insert range.
(_Hashtable(_InputIterator, _InputIterator, size_type, const
_Hash&, const _Equal&,
const allocator_type&, false_type)): Use _M_insert.
* testsuite/23_containers/unordered_map/cons/56112.cc: Check
how many times conversion
is done.
(test02): New test case.
* testsuite/23_containers/unordered_set/cons/56112.cc: New test.
Tested under Linux x86_64.
Ok to commit ?
No, sorry.
The new test02 function in 23_containers/unordered_map/cons/56112.cc
doesn't compile with libc++ or MSVC either, are you sure that test is
valid? I don't think it is, because S2 is not convertible to
pair<const MoveOnlyKey, int>. None of the pair constructors are
viable, because the move constructor would require two user-defined
conversions (from S2 to pair<MoveOnlyKey, int> and then from
pair<MoveOnlyKey, int> to pair<const MoveOnlyKey, int>). A conversion
sequence cannot have more than one user-defined conversion using a
constructor or converion operator. So if your patch makes that
compile, it's a bug in the new code. I haven't analyzed that code to
see where the problem is, I'm just looking at the test results and the
changes in behaviour.
The new 23_containers/unordered_set/cons/56112.cc test fails for GCC
11 but passes for GCC 12, even without your patch. Is it actually
testing some other change, not this patch, and not the 2013 fix for PR
56112?
.
Yes, I'm not surprised. This is due to this operator on _ValueTypeEnforcer:
constexpr __enable_if_t<std::is_const<__fst_type>::value,
const __mutable_value_t<_Value>&>
operator()(const __mutable_value_t<_Value>& __x) noexcept
{ return __x; }
I thought it was nice to allow move construction in this case.
If the Standard forces a copy in this case I can just remove it.
François
