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

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

Hi

Is this less ambitious version ok ?

Thanks

On 25/05/22 07:09, François Dumont wrote:
> Here is the patch to fix just what is described in PR 105714.
>
>     libstdc++: [_Hashtable] Insert range of types convertible to 
> value_type PR 105714
>
>     Fix insertion of range of types convertible to value_type.
>
>     libstdc++-v3/ChangeLog:
>
>             PR libstdc++/105714
>             * 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.
>             * testsuite/23_containers/unordered_map/insert/105714.cc: 
> New test.
>             * testsuite/23_containers/unordered_set/insert/105714.cc: 
> New test.
>
> Tested under Linux x64, ok to commit ?
>
> François
>
> On 24/05/22 12:31, Jonathan Wakely wrote:
>> On Tue, 24 May 2022 at 11:22, Jonathan Wakely wrote:
>>> On Tue, 24 May 2022 at 11: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.
>>> I meant to include this link showing that libc++ and MSVC reject
>>> test02() as well:
>>>
>>> https://godbolt.org/z/j7E9f6bd4
>> I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105717 for
>> the insertion bug, rather than reopening PR 56112.
>>