Re: [v3 PATCH] Make poisoned hashes SFINAE away the call operator of the hash.

[Jonathan Wakely <jwakely at redhat dot com>]

On 20/01/17 17:00 +0200, Ville Voutilainen wrote:
> --- a/libstdc++-v3/include/bits/unique_ptr.h
> +++ b/libstdc++-v3/include/bits/unique_ptr.h
> @@ -789,10 +789,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     { return !(nullptr < __x); }
>
>   /// std::hash specialization for unique_ptr.
> -  template<typename _Tp, typename _Dp>
> -    struct hash<unique_ptr<_Tp, _Dp>>
> -    : public __hash_base<size_t, unique_ptr<_Tp, _Dp>>,
> -    private __poison_hash<typename unique_ptr<_Tp, _Dp>::pointer>
> +
> +  template<typename _Tp, typename _Dp, bool>
> +    struct __unique_ptr_hash_call_base

Could this deduce the bool automatically? i.e.

 template<typename _Tp, typename _Dp, bool
           = __poison_hash<typename unique_ptr<_Tp, _Dp>::pointer>::__enable_hash_call>
   struct __unique_ptr_hash_call_base

>     {
>       size_t
>       operator()(const unique_ptr<_Tp, _Dp>& __u) const noexcept
> @@ -802,6 +801,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       }
>     };
>
> +  template<typename _Tp, typename _Dp>
> +    struct __unique_ptr_hash_call_base<_Tp, _Dp, false> {};
> +
> +  template<typename _Tp, typename _Dp>
> +    struct hash<unique_ptr<_Tp, _Dp>>
> +    : public __hash_base<size_t, unique_ptr<_Tp, _Dp>>,
> +    private __poison_hash<typename unique_ptr<_Tp, _Dp>::pointer>,
> +    public __unique_ptr_hash_call_base<_Tp, _Dp,
> +      __poison_hash<typename unique_ptr<_Tp, _Dp>::pointer>::
> +        __enable_hash_call>

If the bool is deduced then this would be slightly less complicated:

 template<typename _Tp, typename _Dp>
   struct hash<unique_ptr<_Tp, _Dp>>
   : public __hash_base<size_t, unique_ptr<_Tp, _Dp>>,
     private __poison_hash<typename unique_ptr<_Tp, _Dp>::pointer>,
     public __unique_ptr_hash_call_base<_Tp, _Dp>
   { };

(But when this much effort is needed to define std::hash something
tells me we've taken a wrong turn in the standard.)

This part for std::unique_ptr is the only part that isn't C++17-only,
so this is the part I'm most nervous about. Given that we're in stage
4 and this isn't even fixing something in Bugzilla (even if it is a
real bug), would it be possible to only fix optional and variant for
now? We could revisit the unique_ptr part once we know this approach
doesn't have some other problem that we haven't guessed yet.

Or if this is fixing some regression introduced by the __poison_hash
stuff please put something in BZ for the record.


> diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
> index 85ec91d..85c95b1 100644
> --- a/libstdc++-v3/include/std/optional
> +++ b/libstdc++-v3/include/std/optional
> @@ -951,21 +951,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     { return optional<_Tp> { in_place, __il, std::forward<_Args>(__args)... }; }
>
>   // Hash.
> +
> +  template<typename _Tp, bool>

Could we deduce the bool here too?

> +    struct __optional_hash_call_base
> +  {

This brace (and the rest of the body) should be indented to line up
with "struct"

> +    size_t
> +    operator()(const optional<_Tp>& __t) const
> +      noexcept(noexcept(hash<_Tp> {}(*__t)))
> +    {
> +      // We pick an arbitrary hash for disengaged optionals which hopefully
> +      // usual values of _Tp won't typically hash to.
> +      constexpr size_t __magic_disengaged_hash = static_cast<size_t>(-3333);
> +      return __t ? hash<_Tp> {}(*__t) : __magic_disengaged_hash;
> +    }
> +  };
> +
>   template<typename _Tp>
> -    struct hash<optional<_Tp>> : private __poison_hash<remove_const_t<_Tp>>
> +    struct __optional_hash_call_base<_Tp, false> {};
> +
> +  template<typename _Tp>
> +    struct hash<optional<_Tp>> : private __poison_hash<remove_const_t<_Tp>>,
> +                               public __optional_hash_call_base<_Tp,
> +                                 __poison_hash<remove_const_t<_Tp>>::
> +                                   __enable_hash_call>

The base classes don't need to be indented so much. Maybe like this
instead:

   struct hash<optional<_Tp>>
   : private __poison_hash<remove_const_t<_Tp>>,
     public __optional_hash_call_base<_Tp,
              __poison_hash<remove_const_t<_Tp>>::__enable_hash_call>

Or if the bool parameter can be deduced it's simpler:

   struct hash<optional<_Tp>>
   : private __poison_hash<remove_const_t<_Tp>>,
     public __optional_hash_call_base<_Tp>