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

[Fan You <youfan dot noey at gmail dot com>]

2015-06-21 18:50 GMT+08:00 Tim Shen <timshen@google.com>:
> On Sun, Jun 21, 2015 at 1:34 AM, Fan You <youfan.noey@gmail.com> wrote:
>> Hi,
>>
>> I forked gcc and added a new branch shared_arrays here
>> <https://github.com/Noeyfan/gcc-1/tree/shared_arrays>
>>
>> Few tests regarding constructors, destructor, comparison and operators
>> had been added, bugs fixed and tests all passed. I haven't put them
>> into separate folder, but it will be done.
>>
>> I also tried gcov and most of the __shared_ptr<__libfund> impl had
>> been tested thoroughly.
>>
>> Fan
>
> Quickly looked at __shared_ptr<__libfund_v1<_Tp>, _Lp>; will look at
> the rest parts later.
>
> I used this working draft:
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4335.html#memory.smartptr.shared.const
>
> I ran `git diff ab2a707c83582b85a20079b53f1c8bc19942f5d1
> c7248656569bb0b4549f5c1ed347f7e028a15664` based on your shared_arrays
> branch:
>
> Please remvoe all trailnig spaces and convert all leading spaces to
> tabs, if possible. Please break long lines into multiple ones,
> regarding the 80 columns limitation:
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/source_code_style.html
>
> +      template<typename _Ptr>
> +       using _Convertible
> +         = typename enable_if<is_convertible<_Ptr, _Tp*>::value>::type;
> Usually put '=' at the end of the line, not begining.

The original shared_ptr imp puts '=' here :)

>
> +      using __base_type = __shared_ptr<element_type>;
> _Base_type.
>
> +      template<typename _Tp1, typename _Deleter, typename _Alloc>
> +       __shared_ptr(_Tp1* __p, _Deleter __d, _Alloc __a)
> +       : __base_type(__p, __d, __a)
> +        { }
> This is probably fine, unless "_Tp1(*)[] is convertible to _Tp*"
> cannot imply "_Tp1* is convertible to element_type*", when _Tp is a
> U[].
>
> +      template<typename _Tp1>
> +       __shared_ptr(const __shared_ptr<__libfund_v1<_Tp1>, _Lp>&
> +                    __r, _Tp* __p) noexcept
> +        : __base_type(__r, __p)
> +        { }
> I think this is wrong? Following code should compile:
>     std::experimental::shared_ptr<int[2]> sp(new int[2]);
>     std::experimental::shared_ptr<int[]> spa(sp);
> since [8.2.1] specified "For the purposes of subclause 8.2, a pointer
> type Y* is said to be compatible with a pointer type T* when either Y*
> is convertible to T* or Y is U[N] and T is U cv []. "

I will think about this.

>
> +      // reset
> +      void
> +      reset() noexcept
> +      {
> +        __shared_ptr(nullptr).swap(*this);
> +      }
> Please wrap the one-line definition:
> { __shared_ptr(nullptr).swap(*this); }
>
> +      element_type&
> +      operator[](ptrdiff_t i) const noexcept
> +      {
> +        _GLIBCXX_DEBUG_ASSERT(get() !=0 && i>=0);
> +        // TODO shared_ptr<void> a (new int);
> +        return get()[i];
> +      }
> Please be aware of spaces surrounding operators:
> _GLIBCXX_DEBUG_ASSERT(get() != 0 && i >= 0);

what does this mean?

>
> +      template<typename _Tp1>
> +       __shared_ptr&
> +       operator=(const __shared_ptr<__libfund_v1<_Tp1>, _Lp>& __r) noexcept
> +       {
> +          this->__base_type::operator=(__r);
> +          return *this;
> +       }
> Shouldn't the base of __r be passed in, instead of the __lib_fund version?

Yes, should be

this->__base_type::operator=(__r.__base_type);

So does others.

>
> +      template<class _Tp1>
> +       __shared_ptr&
> +       operator=(__shared_ptr<__libfund_v1<_Tp1>, _Lp>&& __r) noexcept
> +       {
> +          // cannot use std::move here
> +          // cause __base_type _Convertible fail
> +          this->__base_type::operator=(__r);
> +          return *this;
> +       }
> Can you give an example, and why?

I mean to do

struct A { };

struct B : A { };

std::experimental::shared_ptr<A[10]> a;

a = std::experimental::shared_ptr<B[10]> (new B[10]);

without std::move(), it can compile && run successfully.

However, I realised its incorrect to do  a =
std::experimental::shared_ptr<B[10]> (new B[10]); Since B[10] is not
convertible to A[10].

So, here should be:

this->__base_type::operator=(std::move(__r));

>
>
> --
> Regards,
> Tim Shen

Thanks.