[libcxx-commits] [PATCH] D62641: Support arrays in make_shared and allocate_shared (P0674R1)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 8 12:38:37 PST 2021
ldionne added a comment.
Thanks a lot for the patch. I've left a lot of comments with suggestions. Feel free to take or leave my suggestions: it's mostly that I had myself started some work on this, so I felt like I should at least put it somewhere instead of just trashing it. Keep what you want and leave the rest.
================
Comment at: libcxx/include/memory:516-519
template<class T, class... Args>
shared_ptr<T> make_shared(Args&&... args);
template<class T, class A, class... Args>
shared_ptr<T> allocate_shared(const A& a, Args&&... args);
----------------
```
template<class T, class... Args>
shared_ptr<T> make_shared(Args&&... args); // T is not array
template<class T, class A, class... Args>
shared_ptr<T> allocate_shared(const A& a, Args&&... args); // T is not array
template<class T>
shared_ptr<T> make_shared(size_t N); // T is U[] (since C++20)
template<class T, class A>
shared_ptr<T> allocate_shared(const A& a, size_t N); // T is U[] (since C++20)
template<class T>
shared_ptr<T> make_shared(); // T is U[N] (since C++20)
template<class T, class A>
shared_ptr<T> allocate_shared(const A& a); // T is U[N] (since C++20)
template<class T>
shared_ptr<T> make_shared(size_t N, const remove_extent_t<T>& u); // T is U[] (since C++20)
template<class T, class A>
shared_ptr<T> allocate_shared(const A& a, size_t N, const remove_extent_t<T>& u); // T is U[] (since C++20)
template<class T> shared_ptr<T>
make_shared(const remove_extent_t<T>& u); // T is U[N] (since C++20)
template<class T, class A>
shared_ptr<T> allocate_shared(const A& a, const remove_extent_t<T>& u); // T is U[N] (since C++20)
```
================
Comment at: libcxx/include/memory:2716
+template<class _Tp, class = _EnableIf<!is_array_v<_Tp>>>
+void __shared_ptr_destroy_elem(_Tp* __ptr) { __ptr->~_Tp(); }
+
----------------
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 :-)
================
Comment at: libcxx/include/memory:2733
+template <class _Tp, class _Alloc>
+struct __shared_ptr_array
+ : __shared_weak_count
----------------
I would call this something like `__shared_ptr_unbounded_array_holder` or similar, to make it clear that this handles unbounded arrays only (see my suggestion to have two separate control blocks below).
================
Comment at: libcxx/include/memory:2736-2738
+ using _Info = __shared_ptr_array_info<_Alloc>;
+ using _InfoBlock = typename std::aligned_storage<sizeof(_Info), alignof(_Info)>::type;
+ using _DataBlock = typename std::aligned_storage<sizeof(_Tp), alignof(_Tp) >::type;
----------------
Why not only have a member of type `_Info`? Or even better, just have
```
[[no_unique_address]] _Alloc __alloc_;
size_t __size_;
union { // make sure the default constructor doesn't initialize __data_
_Tp __data_[1]; // flexible array member trick
};
```
inline in the control block definition?
================
Comment at: libcxx/include/memory:2759-2771
+ void __fill_data(_Et __element) {
+ _Tp *__ptr = __get_elem_begin();
+ size_t __size = __get_size();
+ for (size_t __i = 0; __i < __size; ++__i, ++__ptr)
+ ::new(static_cast<void*>(__ptr)) _Tp(__element);
+ }
+
----------------
Those need to use Allocator construction. I would suggest a more generic algorithm like:
```
// Given an allocator and a range delimited by [first, first + n), initializes
// the range [first, first + n) from left to right using the allocator's construct.
//
// Each element is initialized using allocator_traits construction, and the given
// arguments are passed to construction. The arguments are not perfect-forwarded
// because that could set us up for double-moves. If the elements of the range
// are themselves C-style arrays, they are recursively constructed in the same
// manner.
//
// If an exception is thrown, the initialized elements are destroyed in reverse
// order of initialization using allocator_traits destruction. This requires that
// the iterator type be a BidirectionalIterator.
//
// 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 __uninitialized_allocator_construct_multidimensional_n(_Alloc const& __alloc, _BidirectionalIterator __first, size_t __n, _Args&& ...__args) {
using _ValueType = typename iterator_traits<_BidirectionalIterator>::value_type;
using _ValueAlloc = typename __allocator_traits_rebind<_Alloc, _ValueType>::type;
auto __idx = __first;
_ValueAlloc __value_alloc(__alloc);
#ifndef _LIBCPP_NO_EXCEPTIONS
try {
#endif
for (; __n > 0; (void)++__idx, --__n) {
if constexpr (is_array_v<_ValueType>) {
size_t __size = _VSTD::distance(_VSTD::begin(*__idx), _VSTD::end(*__idx));
__uninitialized_allocator_construct_multidimensional_n(__value_alloc, _VSTD::begin(*__idx), __size, __args...);
} else {
allocator_traits<_ValueAlloc>::construct(__value_alloc, _VSTD::addressof(*__idx), __args...);
}
}
#ifndef _LIBCPP_NO_EXCEPTIONS
} catch (...) {
_VSTD::__allocator_destroy_multidimensional(__first, __idx);
throw;
}
#endif
}
```
================
Comment at: libcxx/include/memory:2777
+ _Tp *__ptr = __get_elem_begin();
+ for (; __i < __size; ++__i, ++__ptr) {
+ __shared_ptr_destroy_elem(__ptr);
----------------
You need to destroy the elements in reverse order of construction. If you use the algorithm I suggested above, it should suffice to say:
```
_VSTD:: __allocator_destroy_multidimensional(__get_elem_begin(), __get_elem_end());
```
or something along those lines.
================
Comment at: libcxx/include/memory:3498
-template<class _Tp, class ..._Args, class = _EnableIf<!is_array<_Tp>::value> >
+#if _LIBCPP_STD_VER > 17
+
----------------
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
```
================
Comment at: libcxx/include/memory:3526
+{
+ return __allocate_shared_array<_Tp>(__a, extent_v<_Tp>, __u);
+}
----------------
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.
================
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
+
----------------
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.
================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:203
"name": "__cpp_lib_shared_ptr_arrays",
- "values": { "c++17": int(201611) }, # "c++20": int(201707) # Enable this when we support arrays in std::make_shared
+ "values": { "c++17": int(201707) },
"headers": ["memory"],
----------------
We need to add `"c++20": 201707` and leave the c++17 entry as-is.
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