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

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

2015-06-21 23:06 GMT+08:00 Fan You <youfan.noey@gmail.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.

Is it a good idea to change _Convertible alias to _Compatible in base.h?

      template <typename _Tp1>
        struct _is_compatible
        {
          bool value = (is_array<_Tp1>::value && is_array<_Tp>::value
                                 && (is_same<typename
remove_extent<_Tp1>::type, element_type>::value))
                               || (is_convertible<_Tp1, element_type*>::value);
        };

      template<typename _Tp1>
        using _Compatible
          = typename enable_if<_is_compatible<_Tp1>().value>::type;
>
>>
>> +      // 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.