Re: [PATCH] Hashtable PR96088

[Jonathan Wakely <jwakely.gcc@gmail.com>]

On Fri, 21 May 2021, 07:31 François Dumont via Libstdc++, <
libstdc++@gcc.gnu.org> wrote:

> On 20/05/21 6:44 pm, Jonathan Wakely wrote:
> > On 06/05/21 22:03 +0200, François Dumont via Libstdc++ wrote:
> >> Hi
> >>
> >>     Considering your feedback on backtrace in debug mode is going to
> >> take me some time so here is another one.
> >>
> >>     Compared to latest submission I've added a _Hash_arg_t partial
> >> specialization for std::hash<>. It is not strictly necessary for the
> >> moment but when we will eventually remove its nested argument_type it
> >> will be. I also wonder if it is not easier to handle for the
> >> compiler, not sure about that thought.
> >
> > The std::hash specializations in libstdc++ define argument_type, but
> > I'm already working on one that doesn't (forstd::stacktrace).
> >
> > And std::hash<acme::ProgramDefinedType> can be specialized by users,
> > and is not required to provide argument_type.
> >
> > So it's already not valid to assume that std::hash<T>::argument_type
> > exists.
>
> Yes, I know that the plan is to get rid of argument_type. But as long as
> it is there we can still use it even if I didn't realize that you were
> already in the process of removing it so soon.
>

I'm adding a new specialization for a C++23 type, and not adding the nested
types.



>
> >
> >> @@ -850,9 +852,56 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >>     iterator
> >>     _M_emplace(const_iterator, false_type __uks, _Args&&... __args);
> >>
> >> +      template<typename _Kt, typename _Arg, typename _NodeGenerator>
> >> +    std::pair<iterator, bool>
> >> +    _M_insert_unique(_Kt&&, _Arg&&, const _NodeGenerator&);
> >> +
> >> +      // Detect nested argument_type.
> >> +      template<typename _Kt, typename _Ht, typename = __void_t<>>
> >> +    struct _Hash_arg_t
> >> +    { typedef _Kt argument_type; };
> >> +
> >> +      // std::hash
> >> +      template<typename _Kt, typename _Arg>
> >> +    struct _Hash_arg_t<_Kt, std::hash<_Arg>>
> >> +    { typedef _Arg argument_type; };
> >> +
> >> +      // Nested argument_type.
> >> +      template<typename _Kt, typename _Ht>
> >> +    struct _Hash_arg_t<_Kt, _Ht,
> >> +              __void_t<typename _Ht::argument_type>>
> >> +    { typedef typename _Ht::argument_type argument_type; };
> >> +
> >> +      // Function pointer.
> >> +      template<typename _Kt, typename _Arg>
> >> +    struct _Hash_arg_t<_Kt, std::size_t(*)(const _Arg&)>
> >> +    { typedef _Arg argument_type; };
> >> +
> >> +      template<typename _Kt,
> >> +           typename _ArgType
> >> +         = typename _Hash_arg_t<_Kt, _Hash>::argument_type>
> >> +    static typename conditional<
> >> +      __is_nothrow_convertible<_Kt, _ArgType>::value, _Kt&&,
> >> key_type>::type
> >
> > Please use __conditional_t<...> here instead of
> > typename conditional<...>::type.
> >
> > The purpose of the _Hash_arg_t type is to determine whether invoking
> > the hash function with _Kt&& can throw, right?
>
> No, the purpose of _Hash_arg_t is to find out what is the argument type
> of the _Hash functor. With this info I can check if invoking that
> functor is going to instantiate a temporary using a throwing operation.


Right, that's what I mean.


> If so, I create a temporary at _Hashtable code level and move it to its
> final storage place when needed.
>
> The tricky part is that _Hash can accept different argument types, for
> the moment I just do not create a temporary in this case.
>
> >
> > And if it can throw, you force a conversion early, and if it can't,
> > you don't do the conversion.
> >
> > Can't you use __is_nothrow_invocable<_Hash&, _Kt> for that, instead of
> > this fragile approach?
> >
> I think I already try but I'll check.
>
> I fear that __is_nothrow_invocable<_Hash&, _Kt> tells if the chosen
> operator()(const _Arg&) is noexcept qualified.


No.

Not if the conversion
> from _Kt to _Arg is noexcept.
>


It does exactly what you need.


>
>