[GSoC] Extend shared_ptr to support array, update 2

[Jonathan Wakely]

On 04/04/15 20:26 -0400, Fan You wrote:
>Hello,
>
>Things I've tried recently, by adding specialization for
>__shared_ptr<__libfund_v1<T>>
> - [8.2.1.1] shared_ptr constructors.
> - [8.2.1.2] shared_ptr observers.
> - Change element_type in __weak_ptr to remove_extent<_Tp>::type;
> - According to
><http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3641.html>,
>I overload make_shared and allocate_shared with an extra parameter
>(size_t __size) to support allocate memory for array type. However,
>it's not in the standard, am I allow to do this?

Since you're only adding them to namespace std::experimental we can be
a bit more flexible on what is added. I suggest following the proposal
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3939.html
(which has not been accepted into the Library Fundamentals TS).

> - And I am also worried about the support for N-dimension array. The
>deleter seems to be fine, but something like shared_ptr<T[N][]...>,
>shared_ptr<T[][N]...> or shared_ptr<T[][]...> may or may not cause
>extra works. Or should I just not consider these for now?

It might be worth considering them, just to make sure you aren't going
to create problems for yourself later, but I think they should Just
Work.

>@@ -627,6 +629,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	    }
> 	}
>
>+      template<typename _Tp, typename _Alloc, typename... _Args>
>+	__shared_count(_Sp_make_shared_tag, _Tp*, const _Alloc& __a,
>+		       size_t __size, _Args&&... __args)
>+	: _M_pi(0)
>+	{
>+	  typedef _Sp_counted_ptr_inplace<_Tp, _Alloc, _Lp> _Sp_cp_type;
>+	  typedef typename allocator_traits<_Alloc>::template
>+	    rebind_traits<_Sp_cp_type> _Alloc_traits;
>+	  typename _Alloc_traits::allocator_type __a2(__a);
>+	  _Sp_cp_type* __mem = _Alloc_traits::allocate(__a2, __size);

This allocates space for an array of _Sp_counted_ptr_inplace objects,
which is not the same as a single _Sp_counted_ptr_inplace and an array
of _Tp[__size].

>+	  __try
>+	    {
>+	      _Alloc_traits::construct(__a2, __mem, std::move(__a),
>+		    std::forward<_Args>(__args)...);
>+	      _M_pi = __mem;
>+	    }
>+	  __catch(...)
>+	    {
>+	      _Alloc_traits::deallocate(__a2, __mem, __size);
>+	      __throw_exception_again;
>+	    }
>+	}
>+
> #if _GLIBCXX_USE_DEPRECATED
>       // Special case for auto_ptr<_Tp> to provide the strong guarantee.
>       template<typename _Tp>
>@@ -1175,6 +1200,123 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       __shared_count<_Lp>  _M_refcount;    // Reference counter.
>     };
>
>+  //new array  approach
>+  template <typename _Tp>
>+    struct __libfund_v1 {using type = _Tp;};
>+
>+  //array support revised
>+  template<typename _Tp, _Lock_policy _Lp, unsigned N>
>+    class __shared_ptr<__libfund_v1<_Tp[N]>, _Lp>

My suggestion was to use the __libfund_v1 tag type for all types, not
only array types:

  template<typename _Tp, _Lock_policy _Lp>
    class __shared_ptr<__libfund_v1<_Tp>, _Lp>
    {

Then you make that specialization meet the requirements of the TS,
including using remove_extent, and performing the right SFINAE
constraints on conversions.

That means the existing __shared_ptr<_Tp> is unchanged, and
__shared_ptr<__libfund_v1<_Tp>> meets all the requirements of
std::experimental::shared_ptr for array types and non-array types.

>+    {
>+    public:
>+      using element_type = typename __libfund_v1<_Tp[N]>::type;

This should be using remove_extent here.



>+      template<typename _Tp1>
>+        explicit __shared_ptr(_Tp1* __p)
>+        : _M_ptr(__p), _M_refcount(__p, _M_del) // default deleter

I still don't understand why you have the _M_del member here.

It just wastes space in every object. If the user provides a custom
deleter, it isn't used, and if they don't provide a custom deleter
then _M_refcount will store a copy, so _M_del is never needed!

Just pass a default-constructed object to _M_refcount's constructor:

>+        : _M_ptr(__p), _M_refcount(__p, _D_Deleter{})

Now you don't need the _M_del member.

[...]

I'd prefer this to have a better name, maybe _Array_deleter:

>+      struct _D_Deleter
>+      {
>+        void
>+        operator()(_Tp const *__p)

This member function should be const.


>@@ -1334,12 +1476,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       return __shared_ptr<_Tp, _Lp>();
>     }
>
>-
>   template<typename _Tp, _Lock_policy _Lp>
>     class __weak_ptr
>     {
>     public:
>-      typedef _Tp element_type;
>+
>+      using element_type = typename remove_extent<_Tp>::type;

This changes the behaviour of std::weak_ptr<T[N]> so I think you also
need a partial specialization of __weak_ptr<__libfund_v1<_Tp>>.

>diff --git a/experimental/memory b/experimental/memory
>new file mode 100644
>index 0000000..1c2328f
>--- /dev/null
>+++ b/experimental/memory
>@@ -0,0 +1,120 @@
>+#include <memory>
>+
>+namespace std {
>+namespace experimental {
>+inline namespace fundamentals_v1 {
>+
>+  template<typename _Tp>
>+    class shared_ptr;

It should be possible to use std::experimental::shared_ptr<int> as
well as std::experimental::shared_ptr<int[]>, so you need to define
the primary template, not only the partial specializations for arrays.

>+  //for non default lock policy
>+  template<typename _Tp, _Lock_policy _Lp = __default_lock_policy>
>+    using __shared_ptr = std::__shared_ptr<_Tp, _Lp>;

Nice.

>+  template<typename _Tp, unsigned N>
>+    class shared_ptr<_Tp[N]> : public __shared_ptr<__libfund_v1<_Tp[N]>>

As stated above, std::experimental::shared_ptr<int> should work too,
so I was thinking that you would always use the __libfund_v1 tag type
i.e.

  template<typename _Tp, unsigned N>
    class shared_ptr : public __shared_ptr<__libfund_v1<_Tp>>

This will be used for all types, even when _Tp is not an array type.

>+    {
>+    public:
>+      typedef typename remove_extent_t<_Tp> element_type;

For std::experimental::__shared_ptr<_Tp[]> to work correctly the
remove_extent_t must be used in the base class, or done by the
__libfund_v1 tag type as shown above.

>+
>+      constexpr shared_ptr() noexcept = default;
>+
>+      template<typename _Tp1>
>+        explicit shared_ptr(_Tp1* __p) : __shared_ptr<__libfund_v1<_Tp[N]>>(__p) { }

It probably makes sense to define a typedef to refer to the base type:

      using __base_type = __shared_ptr<__libfund_v1<_Tp>>;

That will simplify all the constructors.


Overall this is looking good, thanks for working on it.