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

[Tim Shen <timshen at google dot 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.

+      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 []. "

+      // 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);

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

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


-- 
Regards,
Tim Shen