[PATCH][GSoC] Extend shared_ptr to support arrays

Jonathan Wakely jwakely@redhat.com
Thu Jun 11 16:15:00 GMT 2015


On 11/06/15 23:32 +0800, Fan You wrote:
>Hi,
>
>This is my first patch for GSoC project: extend shared_ptr to support
>arrays.
>
>Changes are made in these files:
>
>* libstdc++-v3/include/bits/shared_ptr_base.h : Renamed _Sp_counted_deleter
>to _Sp_counted_array, changed _shared_count construct from _Sp_counted_ptr
>to _Sp_counted_array.

Why?

As far as I can see it has nothing to do with arrays.

>* libstdc++-v3/include/experimental/memory : add shared_ptr into
>fundamentals_v1
>
>Is it too long for reviewing? Should I detach them into different patches?
>Any comments?

General comments:

Please fix the lines that consist of nothing but whitespace, they
should just be empty lines.

You're going to need tests!


Comments on changes to include/bits/shared_ptr_base.h:

This type should not be declared when including <memory>:

+  namespace experimental{
+  inline namespace fundamentals_v1 {
+    template<typename _Tp>
+      class enable_shared_from_this;
+  }
+  }
+

You will need to find another way to make
experimental::enable_shared_from_this work, without declaring it in
<bits/shared_ptr.h> (e.g. add a new type with a reserved name and make
experimental::enable_shared_from_this use that, and only declare
experimental::enable_shared_from_this in <experimental/memory>).


Please review the patch to ensure you are not reverting my recent
changes on trunk e.g.

-#if __cpp_rtti
-       // _GLIBCXX_RESOLVE_LIB_DEFECTS
-       // 2400. shared_ptr's get_deleter() should use addressof()
-        return __ti == typeid(_Deleter)
-         ? std::__addressof(_M_impl._M_del())
-         : nullptr;
+#ifdef __GXX_RTTI
+       return __ti == typeid(_Deleter) ? &_M_impl._M_del() : nullptr;
 #else
         return nullptr;
 #endif

This should definitely not be in the patch, it's a regression.

You've also reverted changes to _Sp_counted_deleter::_M_destroy().

Maybe I'm wrong, but I don't tink it's necessary to provide three
partial specializations:

+  // alias for original __shared_ptr
+  template <typename _Tp, _Lock_policy _Lp>
+    class __shared_ptr<__libfund_v1<_Tp>, _Lp> : public __shared_ptr<_Tp, _Lp>
+    {
...
+  // array support
+  template<typename _Tp, _Lock_policy _Lp, unsigned N>
+    class __shared_ptr<__libfund_v1<_Tp[N]>, _Lp>
+    {
...
+  template<typename _Tp, _Lock_policy _Lp>
+    class __shared_ptr<__libfund_v1<_Tp[]>, _Lp>

Isn't it possible to provide just one, which handles all three cases?

  // Implementation of std::experimental::shared_ptr
  template <typename _Tp, _Lock_policy _Lp>
    class __shared_ptr<__libfund_v1<_Tp>, _Lp>
    {
      using _Deleter_type = typename conditional<is_array<_Tp>::value,
        _Normal_deleter, _Array_deleter>::type;

    public:
      using element_type = typename remove_extent<_Tp>::type;

      template<typename _Tp1>
        __shared_ptr(_Tp1* __p)
        : _M_ptr(__p), _M_refcount(__p, Deleter_type{})


Alternatively, have one partial specialization for non-arrays (which
derives from __shared_ptr<_Tp>, as you have now) and another for
arrays. That could be done like so:

  template<typename _Tp, bool = is_array<_Tp>::value>
    struct __libfund_v1 { };

  // Implementation of std::experimental::shared_ptr for non-arrays
  template <typename _Tp, _Lock_policy _Lp>
    class __shared_ptr<__libfund_v1<_Tp, false>, _Lp>
    : public __shared_ptr<_Tp>
    {
      // ...
    }

  // Implementation of std::experimental::shared_ptr for arrays
  template <typename _Tp, _Lock_policy _Lp>
    class __shared_ptr<__libfund_v1<_Tp, true>, _Lp>
    : public __shared_ptr<_Tp>
    {
      // ...
    }

It should be possible for the same partial specialization to work for
shared_ptr<T[]> and shared_ptr<T[N]>.


Even if that isn't possible, this is wrong:

+      using element_type = _Tp[N];

And in your new _weak_ptr<__libfund_v1<_Tp>, _Lp> specialization this
is wrong:

+      using element_type = _Tp;



It looks to me as though calling experimental::__weak_ptr<T>::lock()
will return the wrong type of __shared_ptr. That might not be
important.


Comments on changes to include/experimental/memory:

I don't see a definition of experimental::weak_ptr.


The way you've defined experimental::__shared_ptr means it doesn't
support arrays (apologies if you've done what I suggested ... it
obviously wasn't right).

I think it should be something like this:

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

i.e. experimental::__shared_ptr<T[]> should use the partial
specialization for arrays.

Then experimental::shared_ptr can just use that:

+  template<typename _Tp>
+    class shared_ptr : public __shared_ptr<_Tp>
+    {





More information about the Libstdc++ mailing list