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