[libcxx-commits] [PATCH] D62641: Support arrays in make_shared and allocate_shared (P0674R1)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Thanks for those suggestions, Louis! Going to add some more tests over the next few days.



================
Comment at: libcxx/include/memory:2716
+template<class _Tp, class = _EnableIf<!is_array_v<_Tp>>>
+void __shared_ptr_destroy_elem(_Tp* __ptr) { __ptr->~_Tp(); }
+
----------------
ldionne wrote:
> This would need to use Allocator-destruction. In the patch I had started working on, I had this:
> 
> ```
> // Given an allocator and a range delimited by [first, last), destroys every
> // element in the range [first, last) FROM RIGHT TO LEFT using allocator
> // destruction. If elements are themselves C-style arrays, they are recursively
> // destroyed in the same manner.
> //
> // This function assumes that destructors do not throw.
> //
> // The allocator does not need to already be bound to the element type of the
> // range -- it is rebound appropriately by this function.
> template<class _Alloc, class _BidirectionalIterator, class ..._Args>
> _LIBCPP_HIDE_FROM_ABI
> void __allocator_destroy_multidimensional(_Alloc const& __alloc, _BidirectionalIterator __first, _BidirectionalIterator __last) noexcept {
>     using _ValueType = typename iterator_traits<_BidirectionalIterator>::value_type;
>     using _ValueAlloc = typename __allocator_traits_rebind<_Alloc, _ValueType>::type;
> 
>     if (__first == __last) {
>         return;
>     }
>     _ValueAlloc __value_alloc(__alloc);
>     auto __destroy = [&](_BidirectionalIterator __it) {
>         if constexpr (is_array_v<_ValueType>) {
>             __allocator_destroy_multidimensional(__value_alloc, _VSTD::begin(*__it), _VSTD::end(*__it));
>         } else {
>             allocator_traits<_ValueAlloc>::destroy(__value_alloc, _VSTD::addressof(*__it));
>         }
>     };
>     for (--__last; __last != __first; --__last) {
>         __destroy(__last);
>     }
>     // handle __first, which is skipped by the loop above
>     __destroy(*__first);
> }
> ```
> 
> If you like it, feel free to add it to your patch. I had placed it in `__memory/utilities.h` since it was meant to be a general purpose helper function. Also, it's not really tested :-)
Good point. Added these two functions as members with a few tweaks. 

>  I had placed it in __memory/utilities.h since it was meant to be a general purpose helper function. 

I can't think of another place we'd want to use these functions, so I'm going to keep them as more specific member functions. It might make sense to break all the shared_ptr stuff out into its own header at some point, though. 


================
Comment at: libcxx/include/memory:3498
 
-template<class _Tp, class ..._Args, class = _EnableIf<!is_array<_Tp>::value> >
+#if _LIBCPP_STD_VER > 17
+
----------------
ldionne wrote:
> You seem to be missing `make_shared` for arrays.
> 
> I had started working on a similar patch, and this is what I had. Feel free to cherry-pick stuff from it if it's useful:
> 
> ```
> #if _LIBCPP_STD_VER > 17
> template<class _Tp, class _Alloc, class ..._Args>
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> __allocate_shared_unbounded_array_impl(const _Alloc& __a, size_t __n, _Args const& ...__args)
> {
>     ...
> }
> 
> template<class _Tp, class _Alloc, class ..._Args>
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> __allocate_shared_bounded_array_impl(const _Alloc& __a, size_t __n, _Args const& ...__args)
> {
>     ...
> }
> 
> template<class _Tp, class _Alloc, class = _EnableIf<is_unbounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> allocate_shared(const _Alloc& __a, size_t __n)
> {
>     return _VSTD::__allocate_shared_unbounded_array_impl<_Tp>(__a, __n);
> }
> 
> template<class _Tp, class = _EnableIf<is_unbounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> make_shared(size_t __n)
> {
>     return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), __n);
> }
> 
> 
> template<class _Tp, class _Alloc, class = _EnableIf<is_bounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> allocate_shared(const _Alloc& __a)
> {
>     return _VSTD::__allocate_shared_bounded_array_impl<_Tp>(__a);
> }
> 
> template<class _Tp, class = _EnableIf<is_bounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> make_shared()
> {
>     return _VSTD::allocate_shared<_Tp>(allocator<_Tp>());
> }
> 
> 
> template<class _Tp, class _Alloc, class = _EnableIf<is_unbounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> allocate_shared(const _Alloc& __a, size_t __n, const remove_extent_t<_Tp>& __t)
> {
>     return _VSTD::__allocate_shared_unbounded_array_impl<_Tp>(__a, __n, __t);
> }
> 
> template<class _Tp, class = _EnableIf<is_unbounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> make_shared(size_t __n, const remove_extent_t<_Tp>& __t)
> {
>     return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), __n, __t);
> }
> 
> 
> template<class _Tp, class _Alloc, class = _EnableIf<is_bounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> allocate_shared(const _Alloc& __a, const remove_extent_t<_Tp>& __t)
> {
>     return _VSTD::__allocate_shared_bounded_array_impl<_Tp>(__a, __t);
> }
> 
> template<class _Tp, class = _EnableIf<is_bounded_array<_Tp>::value> >
> _LIBCPP_HIDE_FROM_ABI
> shared_ptr<_Tp> make_shared(const remove_extent_t<_Tp>& __t)
> {
>     return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), __t);
> }
> #endif // > C++17
> ```
> You seem to be missing make_shared for arrays.

Aha! I actually took this from your earlier patch: make_shared can be implemented in terms of allocate_shared, so it can just be one function template with no constraints that calls out to all the various allocate_shared overloads.

I think the way I have this now composes really nicely, so unless you see a reason to use something else, I'm going to keep this. 


================
Comment at: libcxx/include/memory:3526
+{
+    return __allocate_shared_array<_Tp>(__a, extent_v<_Tp>, __u);
+}
----------------
ldionne wrote:
> I think it would be useful to have a separate control block for arrays with a known size. That way, we can keep the knowledge of the bounds static.
I considered this, but I don't think it will actually give us any performance benefits, because we have to dynamically allocate it using the provided allocator either way. 


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/create_array.pass.cpp:9
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
----------------
ldionne wrote:
> I would like to see more tests for:
> 
> 1. Multidimensional construction and destruction, and
> 2. Making sure that we construct and destroy elements in the correct order (both in normal circumstances but also when an exception is thrown while we're initializing the array).
> 
> I know this is complicated to test, but I think it is important. It may make sense to split tests into multiple files to make it easier to understand. Your call.
Improved this a little, but I'm going to upload more tests shortly. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62641/new/

https://reviews.llvm.org/D62641