Re: [PATCH][_Hashtable] Fix insertion of range of type convertible to value_type PR 56112

["François Dumont" <frs.dumont@gmail.com>]

On 24/05/22 12:18, Jonathan Wakely wrote:
> On Thu, 5 May 2022 at 18:38, François Dumont via Libstdc++
> <libstdc++@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