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

[Jonathan Wakely <jwakely@redhat.com>]

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?