This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][GSoC] Extend shared_ptr to support arrays


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>
+    {




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]