Extending make_shared to Support Arrays (Partially) - P0674R1

[zoecarver]

Summary

Hi there,

I am a new contributor to libc++, so this patch will probably need some help. This patch incorporates some of the details highlighted in P0674R1. If all goes well, I will submit another one (or update this one) with the rest of the changes.

Example

shared_ptr<double[1024]> p = make_shared<double[1024]>();

Questions

1. Is it okay to define __shared_ptr_emplace__destroy, a helper function, where and how I did?
2. Was removing is_constructible<_Tp, _Args...>::value okay (see comment in code)?
3. What are the next steps? Do I need to write more tests? Do I need to write some documentation?

[zoecarver]

Add tests and fix naming of _N (_N -> __n).

[zoecarver]

@EricWF and @ldionne friendly ping -- could you review this patch?

[mclow.lists]

include/memory

3705

Is there a reason you can't use _VSTD::destroy_at?

3712

Is there a reason you can't use _VSTD::destroy_n?

4358

It's checking to see if you can construct a _Tp from that list of arguments.

test/libcxx/memory/shared_ptr_array.pass.cpp

26

We actually have an ASSERT_SAME_TYPE macro in "test_macros.h"

28

I think you want to test that 64 constructors got called, and (afterwards) that 64 destructors got called.

[zoecarver]

include/memory

3705

I need to destroy it differently if it is an array.

Also, just out of curiosity, I thought _VSTD:: was synonymous to std:: is that true?

[zoecarver]

Is destroy_at / destroy_n prefured to simply calling the destructor?

[zoecarver]

include/memory

4358

hmm -- just went back and enabled this, seemed to be fine. Not sure why I disabled it in the first place ¯\_(ツ)_/¯

test/libcxx/memory/shared_ptr_array.pass.cpp

28

Good idea, is there a macro for this or something?

[zoecarver]

test/libcxx/memory/shared_ptr_array.pass.cpp

28

Nevermind: I figured it out.

[zoecarver]

I added a test to make sure the right number of constructors/destructors were called.

I could not get ASSERT_SAME_TYPE working, I looked at some examples but eventually gave up. From the snippet I gave can you see what is wrong?

Thanks again for helping me through this process :)

[ldionne]

This is a good start, but please make sure you implement and test the full paper.

include/memory

3707

You have two underscores between emplace and destroy.

3709

This here should be _VSTD::destroy_at(&t), and t needs to be mangled.

3716

I would use this instead:

_VSTD::destroy(_VSTD::begin(t), _VSTD::end(t))

Also, t needs to be mangled to __t.

4718

You'll need other overloads as outlined in http://wg21.link/p0674. Please add them in this review, otherwise it's too difficult to tell whether the paper is being implemented correctly across reviews.

test/libcxx/memory/shared_ptr_array.pass.cpp

24

test_constructors (typo)

53

https://wandbox.org/permlink/YuADEmFtbD8ZlNS3

The result of dereferencing a pointer like **pi is a lvalue (int& in this case).

[zoecarver]

Thanks for the edits, I will fix them now.

include/memory

4718

Fair enough, I will implement those in this review then.

@ldionne I am having trouble with how to implement arrays with unknown length, could you take a look?

[zoecarver]

Sorry about the delay updating this patch. I think I got everything except the additional overloads.

I spent some time trying to add the other overloads. Unless I am not seeing something (which is very possible) I think the shared_ptr class might have to have some serious work in order to support array types with undefined length.

include/memory

3709

Could be wrong about this but I think that this needs to work in C++11 but destroy wasn't introduced until C++17.

3716

Same as above comment.

test/libcxx/memory/shared_ptr_array.pass.cpp

24

How did I not see this 🤦‍♂️

53

Very helpful thank you.

[zoecarver]

Abandoned in favor of D62641

include/memory

 private:
     virtual void __on_zero_shared() _NOEXCEPT;
     virtual void __on_zero_shared_weak() _NOEXCEPT;
 public:
     _LIBCPP_INLINE_VISIBILITY
     _Tp* get() _NOEXCEPT {return _VSTD::addressof(__data_.second());}
 };
 
+template <typename _Tp>
+void
+__shared_ptr_emplace__destroy(_Tp& t)
+{
+    t.~_Tp();
+}
+
+template <typename _Tp, std::size_t __n>
+void
+__shared_ptr_emplace__destroy(_Tp (&t)[__n])
+{
+    for (auto i = __n; i > 0; i--) __shared_ptr_emplace__destroy(t[i]);
+}
+
 template <class _Tp, class _Alloc>
 void
 __shared_ptr_emplace<_Tp, _Alloc>::__on_zero_shared() _NOEXCEPT
 {
-    __data_.second().~_Tp();
+    __shared_ptr_emplace__destroy(__data_.second());
 }
 
 template <class _Tp, class _Alloc>
 void
 __shared_ptr_emplace<_Tp, _Alloc>::__on_zero_shared_weak() _NOEXCEPT
 {
     typedef typename __allocator_traits_rebind<_Alloc, __shared_ptr_emplace>::type _Al;
     typedef allocator_traits<_Al> _ATraits;
shared_ptr<_Tp>::make_shared(_Args&& ...__args)
     return __r;
 }
 
 template<class _Tp>
 template<class _Alloc, class ..._Args>
 shared_ptr<_Tp>
 shared_ptr<_Tp>::allocate_shared(const _Alloc& __a, _Args&& ...__args)
 {
-    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared" );
+    // XXX is it okay to delete the line below? It seems like it is only checking if _Tp is an array.
+    // static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared" );
     typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk;
     typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;
     typedef __allocator_destructor<_A2> _D2;
     _A2 __a2(__a);
     unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
     ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
         _CntrlBlk(__a, _VSTD::forward<_Args>(__args)...);
     shared_ptr<_Tp> __r;

 {
     shared_ptr(__p, __d, __a).swap(*this);
 }
 
 #ifndef _LIBCPP_HAS_NO_VARIADICS
 
 template<class _Tp, class ..._Args>
 inline _LIBCPP_INLINE_VISIBILITY
-typename enable_if
-<
-    !is_array<_Tp>::value,
-    shared_ptr<_Tp>
->::type
 make_shared(_Args&& ...__args)
 {
     return shared_ptr<_Tp>::make_shared(_VSTD::forward<_Args>(__args)...);
 }
 
 template<class _Tp, class _Alloc, class ..._Args>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <

test/libcxx/memory/shared_ptr_array.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+
+// <memory>
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
+#include <memory>
+#include <string>
+
+#include "test_macros.h"
+#include "min_allocator.h"
+#include "test_allocator.h"
+
+template <typename T>
+void test_make_shared()
+{
+    std::shared_ptr<T[64]> foo = std::make_shared<T[64]>();
+    std::shared_ptr<T[64]> bar(foo);
+
+    assert(typeid(**foo) == typeid(T) && "Type is not the same");
+    assert(typeid(**bar) == typeid(T) && "Type is not the same");
+}
+
+void test_values()
+{
+    std::shared_ptr<int[64]> foo = std::make_shared<int[64]>();
+    
+    for (int i = 0; i < 64; i++)
+        (*foo)[i] = i;
+    
+    for (int i = 0; i < 64; i++)
+        assert((*foo)[i] == i && "Incorrect retrieved from array value");    
+}
+
+
+int main()
+{
+    test_make_shared<char>();
+    test_make_shared<int>();
+    test_make_shared<std::string>();
+    
+    test_values();
+}