Re: [GSoC] Extend shared_ptr to support array, update 2

[Jonathan Wakely <jwakely at redhat dot com>]

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.