Re: [PATCH] Implement LWG 2686, hash<error_condition>

[Jonathan Wakely <jwakely at redhat dot com>]

On 23/03/17 17:49 +0000, Jonathan Wakely wrote:
> On 12/03/17 13:16 +0100, Daniel Krügler wrote:
> > The following is an *untested* patch suggestion, please verify.
> >
> > Notes: My interpretation is that hash<error_condition> should be
> > defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please
> > double-check that course of action.
>
> That's right.
>
> > I noticed that the preexisting hash<error_code> did directly refer to
> > the private members of error_code albeit those have public access
> > functions. For consistency I mimicked that existing style when
> > implementing hash<error_condition>.
>
> I see no reason for that, so I've removed the friend declaration and
> used the public member functions.

I'm going to do the same for hash<error_code> too. It can also use the
public members instead of being a friend.


> Although this is a DR, I'm treating it as a new C++17 feature, so I've
> adjusted the patch to only add the new specialization for C++17 mode.
> We're too close to the GCC 7 release to be adding new things to the
> default mode, even minor things like this. After GCC 7 is released we
> can revisit it and decide if we want to enable it for all modes.

We never revisited that, and it's still only enabled for C++17 and up.
I guess that's OK, but we could enabled it for C++11 and 14 on trunk
if we want. Anybody care enough to argue for that?

> Here's what I've tested and will be committing.

> commit 90ca0fd91f5c65af370beb20af06bdca257aaf63
> Author: Jonathan Wakely <jwakely@redhat.com>
> Date:   Thu Mar 23 11:47:39 2017 +0000
>
>    Implement LWG 2686, std::hash<error_condition>, for C++17
>    
>    2017-03-23  Daniel Kruegler  <daniel.kruegler@gmail.com>
>    
>    	Implement LWG 2686, Why is std::hash specialized for error_code,
>    	but not error_condition?
>    	* include/std/system_error (hash<error_condition>): Define for C++17.
>    	* testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>):
>    	Instantiate test for error_condition.
>    	* testsuite/20_util/hash/requirements/explicit_instantiation.cc
>    	(hash<error_condition>): Instantiate hash<error_condition>.
>
> diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error
> index 6775a6e..ec7d25f 100644
> --- a/libstdc++-v3/include/std/system_error
> +++ b/libstdc++-v3/include/std/system_error
> @@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _GLIBCXX_END_NAMESPACE_VERSION
> } // namespace
>
> -#ifndef _GLIBCXX_COMPATIBILITY_CXX0X
> -
> #include <bits/functional_hash.h>
>
> namespace std _GLIBCXX_VISIBILITY(default)
> {
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> +#ifndef _GLIBCXX_COMPATIBILITY_CXX0X
>   // DR 1182.
>   /// std::hash specialization for error_code.
>   template<>
> @@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp);
>       }
>     };
> +#endif // _GLIBCXX_COMPATIBILITY_CXX0X
> +
> +#if __cplusplus > 201402L
> +  // DR 2686.
> +  /// std::hash specialization for error_condition.
> +  template<>
> +    struct hash<error_condition>
> +    : public __hash_base<size_t, error_condition>
> +    {
> +      size_t
> +      operator()(const error_condition& __e) const noexcept
> +      {
> +	const size_t __tmp = std::_Hash_impl::hash(__e.value());
> +	return std::_Hash_impl::__hash_combine(__e.category(), __tmp);

When I changed this from using __e._M_cat (as in Daniel's patch) to
__e.category() I introduced a bug, because the former is a pointer to
the error_category (and error_category objects are unique and so can
be identified by their address) and the latter is the object itself,
so we hash the bytes of an abstract base class instead of hashing the
pointer to it. Oops.

Patch coming up to fix that.

We might want to consider adding a static assertion to _Hash_impl to
stop me doing something silly like that again:

--- a/libstdc++-v3/include/bits/functional_hash.h
+++ b/libstdc++-v3/include/bits/functional_hash.h
@@ -199,12 +199,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
    template<typename _Tp>
      static size_t
      hash(const _Tp& __val)
-      { return hash(&__val, sizeof(__val)); }
+      {
+#if __cplusplus >= 201103L
+       static_assert(std::is_trivially_copyable<_Tp>::value, "");
+#endif
+       return hash(&__val, sizeof(__val));
+      }

    template<typename _Tp>
      static size_t
      __hash_combine(const _Tp& __val, size_t __hash)
-      { return hash(&__val, sizeof(__val), __hash); }
+      {
+#if __cplusplus >= 201103L
+       static_assert(std::is_trivially_copyable<_Tp>::value, "");
+#endif
+       return hash(&__val, sizeof(__val), __hash);
+      }
  };

  // A hash function similar to FNV-1a (see PR59406 for how it differs).

We probably shouldn't be hashing raw bytes of things that can't be
copied with memcpy, right?