diff mbox series

PR libstdc++/90388 fix std::hash<unique_ptr<T,D>> bugs

Message ID 20190510214147.GA18999@redhat.com
State New
Headers show
Series PR libstdc++/90388 fix std::hash<unique_ptr<T,D>> bugs | expand

Commit Message

Jonathan Wakely May 10, 2019, 9:41 p.m. UTC
A disabled specialization should not be callable, so move the function
call operator into a new base class which correctly implements the
disabled hash semantics. For the versioned namespace configuration do
not derive from __poison_hash in the enabled case, as the empty base
class serves no purpose but potentially increases the object size. For
the default configuration that base class must be kept, to preserve
layout.

An enabled specialization should not be unconditionally noexcept,
because the underlying hash object might throw.

	PR libstdc++/90388
	* include/bits/unique_ptr.h (default_delete, default_delete<T[]>):
	Use _Require for constraints.
	(operator>(nullptr_t, const unique_ptr<T,D>&)): Implement exactly as
	per the standard.
	(__uniq_ptr_hash): New base class with conditionally-disabled call
	operator.
	(hash<unique_ptr<T,D>>): Derive from __uniq_ptr_hash.
	* testsuite/20_util/default_delete/48631_neg.cc: Adjust dg-error line.
	* testsuite/20_util/unique_ptr/hash/90388.cc: New test.

Tested powerpc64le-linux, committed to trunk.
commit e8d13e93aacad3587d00b71281da953dc1ad2cfd
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri May 10 22:06:39 2019 +0100

    PR libstdc++/90388 fix std::hash<unique_ptr<T,D>> bugs
    
    A disabled specialization should not be callable, so move the function
    call operator into a new base class which correctly implements the
    disabled hash semantics. For the versioned namespace configuration do
    not derive from __poison_hash in the enabled case, as the empty base
    class serves no purpose but potentially increases the object size. For
    the default configuration that base class must be kept, to preserve
    layout.
    
    An enabled specialization should not be unconditionally noexcept,
    because the underlying hash object might throw.
    
            PR libstdc++/90388
            * include/bits/unique_ptr.h (default_delete, default_delete<T[]>):
            Use _Require for constraints.
            (operator>(nullptr_t, const unique_ptr<T,D>&)): Implement exactly as
            per the standard.
            (__uniq_ptr_hash): New base class with conditionally-disabled call
            operator.
            (hash<unique_ptr<T,D>>): Derive from __uniq_ptr_hash.
            * testsuite/20_util/default_delete/48631_neg.cc: Adjust dg-error line.
            * testsuite/20_util/unique_ptr/hash/90388.cc: New test.

Comments

Ville Voutilainen May 10, 2019, 9:51 p.m. UTC | #1
On Sat, 11 May 2019 at 00:42, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> A disabled specialization should not be callable, so move the function
> call operator into a new base class which correctly implements the
> disabled hash semantics. For the versioned namespace configuration do
> not derive from __poison_hash in the enabled case, as the empty base
> class serves no purpose but potentially increases the object size. For
> the default configuration that base class must be kept, to preserve
> layout.

I continue to not be a fan of the versioned namespace ifdeffery in
this, but I can live with it.
Jonathan Wakely May 10, 2019, 9:55 p.m. UTC | #2
On 11/05/19 00:51 +0300, Ville Voutilainen wrote:
>On Sat, 11 May 2019 at 00:42, Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> A disabled specialization should not be callable, so move the function
>> call operator into a new base class which correctly implements the
>> disabled hash semantics. For the versioned namespace configuration do
>> not derive from __poison_hash in the enabled case, as the empty base
>> class serves no purpose but potentially increases the object size. For
>> the default configuration that base class must be kept, to preserve
>> layout.
>
>I continue to not be a fan of the versioned namespace ifdeffery in
>this, but I can live with it.

The versioned namespace configuration should be as good as we can make
it, with no limitations due to ABI compatibility. Removing redundant
base classes allows more compact class layouts. With current trunk
this type has size 3, in the versioned namespace after my patch it has
size 2 (and if we patched hash<optional<T>> it would have size 1):

template<typename T>
struct HashPtr
: std::hash<T*>,
  std::hash<std::optional<T*>>,
  std::hash<std::optional<T* const>>,
  std::hash<std::unique_ptr<T>>
{
};
Ville Voutilainen May 10, 2019, 9:59 p.m. UTC | #3
On Sat, 11 May 2019 at 00:55, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 11/05/19 00:51 +0300, Ville Voutilainen wrote:
> >On Sat, 11 May 2019 at 00:42, Jonathan Wakely <jwakely@redhat.com> wrote:
> >>
> >> A disabled specialization should not be callable, so move the function
> >> call operator into a new base class which correctly implements the
> >> disabled hash semantics. For the versioned namespace configuration do
> >> not derive from __poison_hash in the enabled case, as the empty base
> >> class serves no purpose but potentially increases the object size. For
> >> the default configuration that base class must be kept, to preserve
> >> layout.
> >
> >I continue to not be a fan of the versioned namespace ifdeffery in
> >this, but I can live with it.
>
> The versioned namespace configuration should be as good as we can make
> it, with no limitations due to ABI compatibility. Removing redundant
> base classes allows more compact class layouts. With current trunk
> this type has size 3, in the versioned namespace after my patch it has
> size 2 (and if we patched hash<optional<T>> it would have size 1):

I understand all of that, but I do question the value of optimizing
the versioned namespace configuration at the cost
of sprinkling such ifdefs into our code.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h
index 6a23669f119..a9e74725dfd 100644
--- a/libstdc++-v3/include/bits/unique_ptr.h
+++ b/libstdc++-v3/include/bits/unique_ptr.h
@@ -66,8 +66,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
        * Allows conversion from a deleter for objects of another type, `_Up`,
        * only if `_Up*` is convertible to `_Tp*`.
        */
-      template<typename _Up, typename = typename
-	       enable_if<is_convertible<_Up*, _Tp*>::value>::type>
+      template<typename _Up,
+	       typename = _Require<is_convertible<_Up*, _Tp*>>>
         default_delete(const default_delete<_Up>&) noexcept { }
 
       /// Calls `delete __ptr`
@@ -102,19 +102,19 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
        * it is undefined to `delete[]` an array of derived types through a
        * pointer to the base type.
        */
-      template<typename _Up, typename = typename
-	       enable_if<is_convertible<_Up(*)[], _Tp(*)[]>::value>::type>
+      template<typename _Up,
+	       typename = _Require<is_convertible<_Up(*)[], _Tp(*)[]>>>
         default_delete(const default_delete<_Up[]>&) noexcept { }
 
       /// Calls `delete[] __ptr`
       template<typename _Up>
-      typename enable_if<is_convertible<_Up(*)[], _Tp(*)[]>::value>::type
+	typename enable_if<is_convertible<_Up(*)[], _Tp(*)[]>::value>::type
 	operator()(_Up* __ptr) const
-      {
-	static_assert(sizeof(_Tp)>0,
-		      "can't delete pointer to incomplete type");
-	delete [] __ptr;
-      }
+	{
+	  static_assert(sizeof(_Tp)>0,
+			"can't delete pointer to incomplete type");
+	  delete [] __ptr;
+	}
     };
 
   /// @cond undocumented
@@ -712,7 +712,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	 unique_ptr<_Tp, _Dp>&) = delete;
 #endif
 
-  /// Equality operator for unique_ptr objects, compares the owned pointers.
+  /// Equality operator for unique_ptr objects, compares the owned pointers
   template<typename _Tp, typename _Dp,
 	   typename _Up, typename _Ep>
     _GLIBCXX_NODISCARD inline bool
@@ -848,23 +848,37 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _GLIBCXX_NODISCARD inline bool
     operator>=(nullptr_t, const unique_ptr<_Tp, _Dp>& __x)
     { return !(nullptr < __x); }
+  // @} relates unique_ptr
+
+  /// @cond undocumented
+  template<typename _Up, typename _Ptr = typename _Up::pointer,
+	   bool = __poison_hash<_Ptr>::__enable_hash_call>
+    struct __uniq_ptr_hash
+#if ! _GLIBCXX_INLINE_VERSION
+    : private __poison_hash<_Ptr>
+#endif
+    {
+      size_t
+      operator()(const _Up& __u) const
+      noexcept(noexcept(std::declval<hash<_Ptr>>()(std::declval<_Ptr>())))
+      { return hash<_Ptr>()(__u.get()); }
+    };
+
+  template<typename _Up, typename _Ptr>
+    struct __uniq_ptr_hash<_Up, _Ptr, false>
+    : private __poison_hash<_Ptr>
+    { };
+  /// @endcond
 
   /// 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>
-    {
-      size_t
-      operator()(const unique_ptr<_Tp, _Dp>& __u) const noexcept
-      {
-	typedef unique_ptr<_Tp, _Dp> _UP;
-	return std::hash<typename _UP::pointer>()(__u.get());
-      }
-    };
+      public __uniq_ptr_hash<unique_ptr<_Tp, _Dp>>
+    { };
 
 #if __cplusplus > 201103L
-
+  /// @relates unique_ptr @{
 #define __cpp_lib_make_unique 201304
 
   /// @cond undocumented
@@ -899,9 +913,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Tp, typename... _Args>
     inline typename _MakeUniq<_Tp>::__invalid_type
     make_unique(_Args&&...) = delete;
+  // @} relates unique_ptr
 #endif
 
-  // @} relates unique_ptr
   // @} group pointer_abstractions
 
 #if __cplusplus >= 201703L
diff --git a/libstdc++-v3/testsuite/20_util/default_delete/48631_neg.cc b/libstdc++-v3/testsuite/20_util/default_delete/48631_neg.cc
index bd81913c651..111a42434db 100644
--- a/libstdc++-v3/testsuite/20_util/default_delete/48631_neg.cc
+++ b/libstdc++-v3/testsuite/20_util/default_delete/48631_neg.cc
@@ -26,4 +26,4 @@  struct D : B { };
 D d;
 std::default_delete<B[]> db;
 typedef decltype(db(&d)) type; // { dg-error "no match" }
-// { dg-error "no type" "" { target *-*-* } 111 }
+// { dg-error "no type" "" { target *-*-* } 112 }
diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/hash/90388.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/hash/90388.cc
new file mode 100644
index 00000000000..0f3b7857797
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/unique_ptr/hash/90388.cc
@@ -0,0 +1,90 @@ 
+// Copyright (C) 2019 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+
+#include <memory>
+
+template<typename Func, typename Arg, typename = void>
+  struct is_callable
+  : std::false_type
+  { };
+
+template<typename Func, typename Arg>
+  struct is_callable<Func, Arg,
+    decltype((void)(std::declval<Func&>()(std::declval<Arg>())))>
+  : std::true_type
+  { };
+
+void
+test01()
+{
+  struct D {
+    struct pointer { };
+    void operator()(pointer) const noexcept { }
+  };
+  static_assert( !is_callable<std::hash<D::pointer>&, D::pointer>::value );
+
+  using UP = std::unique_ptr<int, D>;
+  // [unord.hash]
+  // Disabled specializations of hash are not function object types
+  static_assert( !is_callable<std::hash<UP>&, UP>::value );
+  static_assert( !is_callable<std::hash<UP>&, UP&>::value );
+  static_assert( !is_callable<std::hash<UP>&, const UP&>::value );
+}
+
+struct D {
+  struct pointer { };
+  void operator()(pointer) const noexcept { }
+};
+
+bool operator==(D::pointer, std::nullptr_t) { return false; }
+bool operator!=(D::pointer, std::nullptr_t) { return true; }
+
+namespace std {
+  template<> struct hash<D::pointer> {
+    size_t operator()(D::pointer) const { throw 1; }
+  };
+}
+
+void
+test02()
+{
+  using UP = std::unique_ptr<int, D>;
+  UP p;
+  std::hash<UP> h;
+  try {
+    // [util.smartptr.hash]
+    // The member functions are not guaranteed to be noexcept.
+    h(p);
+    throw "should not reach here";
+  } catch (int) {
+    // Should catch exception here, rather than terminating.
+  }
+
+  // Should still be noexcept if the underlying hash object is:
+  using UP2 = std::unique_ptr<int>;
+  UP2 p2;
+  std::hash<UP2> h2;
+  static_assert( noexcept(h2(p2)), "operator() is noexcept" );
+}
+
+int
+main()
+{
+  test02();
+}