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

[Jonathan Wakely <jwakely@redhat.com>]

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.