[libcxx-commits] [PATCH] D62641: Support arrays in make_shared and allocate_shared (P0674R1)
Zoe Carver via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sun Jan 10 22:53:05 PST 2021
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
More information about the libcxx-commits
mailing list