| Message ID | CAFk2RUY1jWY1=ZFbn14tAtv4POrWVg8JBN2g78mcyGD2qL_teg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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>
On 20 January 2017 at 17:21, Jonathan Wakely <jwakely@redhat.com> wrote: > 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. Sure. I would guesstimate that custom pointer types that aren't hashable should be rare, so hitting that problem is unlikely. I'll remove the unique_ptr bits, will give the deduced-bool a spin and fix the whitespace, and will then send a new patch for review.
diff --git a/libstdc++-v3/include/bits/functional_hash.h b/libstdc++-v3/include/bits/functional_hash.h index 14f7fae..38be172 100644 --- a/libstdc++-v3/include/bits/functional_hash.h +++ b/libstdc++-v3/include/bits/functional_hash.h @@ -60,6 +60,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template<typename _Tp, typename = void> struct __poison_hash { + static constexpr bool __enable_hash_call = false; private: // Private rather than deleted to be non-trivially-copyable. __poison_hash(__poison_hash&&); @@ -69,6 +70,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template<typename _Tp> struct __poison_hash<_Tp, __void_t<decltype(hash<_Tp>()(declval<_Tp>()))>> { + static constexpr bool __enable_hash_call = true; }; // Helper struct for SFINAE-poisoning non-enum types. diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h index a31cd67..f4763a0 100644 --- 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 { 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 __cplusplus > 201103L #define __cpp_lib_make_unique 201304 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> + struct __optional_hash_call_base + { + 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> { using result_type = size_t; using argument_type = optional<_Tp>; - - 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; - } }; /// @} diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 6404fce..0ff8439 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -1273,14 +1273,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION std::forward<_Variants>(__variants)...); } - template<typename... _Types> - struct hash<variant<_Types...>> - : private __detail::__variant::_Variant_hash_base< - variant<_Types...>, std::index_sequence_for<_Types...>> + template<bool, typename... _Types> + struct __variant_hash_call_base { - using result_type = size_t; - using argument_type = variant<_Types...>; - size_t operator()(const variant<_Types...>& __t) const noexcept((is_nothrow_callable_v<hash<decay_t<_Types>>(_Types)> && ...)) @@ -1297,6 +1292,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } }; + template<typename... _Types> + struct __variant_hash_call_base<false, _Types...> {}; + + template<typename... _Types> + struct hash<variant<_Types...>> + : private __detail::__variant::_Variant_hash_base< + variant<_Types...>, std::index_sequence_for<_Types...>>, + public __variant_hash_call_base<( + __poison_hash<remove_const_t<_Types>>:: + __enable_hash_call &&...), _Types...> + { + using result_type = size_t; + using argument_type = variant<_Types...>; + }; + template<> struct hash<monostate> { diff --git a/libstdc++-v3/testsuite/20_util/optional/hash.cc b/libstdc++-v3/testsuite/20_util/optional/hash.cc index ceb862b..297ea2e 100644 --- a/libstdc++-v3/testsuite/20_util/optional/hash.cc +++ b/libstdc++-v3/testsuite/20_util/optional/hash.cc @@ -29,6 +29,12 @@ template<class T> auto f(...) -> decltype(std::false_type()); static_assert(!decltype(f<S>(0))::value, ""); +static_assert(!std::is_callable< + std::hash<std::optional<S>>& + (std::optional<S> const&)>::value, ""); +static_assert(std::is_callable< + std::hash<std::optional<int>>& + (std::optional<int> const&)>::value, ""); int main() { diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/hash/1.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/hash/1.cc index dfe8a69..dcd3f9e 100644 --- a/libstdc++-v3/testsuite/20_util/unique_ptr/hash/1.cc +++ b/libstdc++-v3/testsuite/20_util/unique_ptr/hash/1.cc @@ -44,6 +44,13 @@ template<class T> auto f(...) -> decltype(std::false_type()); static_assert(!decltype(f<Pointer<int>>(0))::value, ""); +static_assert(!std::__is_callable< + std::hash<std::unique_ptr<Pointer<int>, PointerDeleter<int>>>& + (std::unique_ptr<Pointer<int>, PointerDeleter<int>> const&)> + ::value, ""); +static_assert(std::__is_callable< + std::hash<std::unique_ptr<int>>& + (std::unique_ptr<int> const&)>::value, ""); void test01() { diff --git a/libstdc++-v3/testsuite/20_util/variant/hash.cc b/libstdc++-v3/testsuite/20_util/variant/hash.cc index a9ebf33..0a267ab 100644 --- a/libstdc++-v3/testsuite/20_util/variant/hash.cc +++ b/libstdc++-v3/testsuite/20_util/variant/hash.cc @@ -33,6 +33,17 @@ static_assert(!decltype(f<std::variant<S>>(0))::value, ""); static_assert(!decltype(f<std::variant<S, S>>(0))::value, ""); static_assert(decltype(f<std::variant<int>>(0))::value, ""); static_assert(decltype(f<std::variant<int, int>>(0))::value, ""); +static_assert(!std::is_callable< + std::hash<std::variant<S>>&(std::variant<S> const&)>::value, ""); +static_assert(!std::is_callable< + std::hash<std::variant<S, int>>& + (std::variant<S, int> const&)>::value, ""); +static_assert(std::is_callable< + std::hash<std::variant<int>>& + (std::variant<int> const&)>::value, ""); +static_assert(std::is_callable< + std::hash<std::variant<int, int>>& + (std::variant<int, int> const&)>::value, ""); int main() {
Tested on Linux-x64. The approach is a bit ugly, the hashes derive from __poison_hash and also use its nested __enable_hash_call as an argument for another utility base. I prototyped various approaches to try to decrease that duplication, but didn't like any of them (more utility bases, aliases, even passing a lambda that does the concrete hash to a more generic hash in a base), and they certainly didn't make the code any shorter. The reason this patch was done is that K-ballo pointed out that our hashes have a non-SFINAED call operator that uses a constructing-expression in its noexcept-spec - so asking is_callable from a poisoned hash leads to a hard error, including in cases where the user would correctly check both callability and constructibility, but in that order. I steered away from turning the call operators into constrained templates, and chose a conditional base approach instead, because that keeps functions as functions instead of turning them into templates; we do not want to open the possibility that a caller starts providing template arguments explicitly. This patch covers unique_ptr, std::optional and std::variant. We might want to entertain doing this for std::experimental::optional as well. 2017-01-20 Ville Voutilainen <ville.voutilainen@gmail.com> Make poisoned hashes SFINAE away the call operator of the hash. * include/bits/functional_hash.h (__poison_hash::__enable_hash_call): New. * include/bits/unique_ptr.h (__unique_ptr_hash_call_base): New. (hash<unique_ptr<_Tp, _Dp>>): Derive from the new base, move the hash function into that base, use the added enabling flag to SFINAE. * include/std/optional (__optional_hash_call_base): New. (hash<optional<_Tp>>): Derive from the new base, move the hash function into that base, use the added enabling flag to SFINAE. * include/std/variant(__variant_hash_call_base): New. (hash<variant<_Types...>>): Derive from the new base, move the hash function into that base, use the added enabling flag to SFINAE. * testsuite/20_util/optional/hash.cc: Add tests for is_callable. * testsuite/20_util/unique_ptr/hash/1.cc: Likewise. * testsuite/20_util/variant/hash.cc: Likewise.