[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
Mon Jan 11 14:37:03 PST 2021
ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/memory:2690
+ template<class _Et>
+ __shared_ptr_array(_Alloc const&__alloc, size_t __size, _Et const&__element)
+ : __alloc(__alloc), __size(__size) {
----------------
I don't understand the benefit of passing `false_type` here when we want to default construct, instead of just relying on variadics and always passing down some `args...` to `__construct_elements` (and hence to the object's constructors themselves). I think it would be simpler to do that.
Also, I think it makes sense to mark this constructor as `explicit`, and it needs to be marked with `_LIBCPP_HIDE_FROM_ABI`.
================
Comment at: libcxx/include/memory:2710
+ _LIBCPP_HIDE_FROM_ABI
+ void __destroy_elements(_ValueType *__first, int __n) noexcept {
+ using _ValueAlloc = typename __allocator_traits_rebind<_Alloc, _ValueType>::type;
----------------
I see you're using an `int` here, but what if construction fails at a very large index, the `size_t` to `int` narrowing could be problematic. Is there a reason why you're using `int`?
================
Comment at: libcxx/include/memory:2753-2758
+ // If we threw in the lowest level of the construct function, we need to include this
+ // array in the destruction. If this function constructed sub-arrays, then they have
+ // already been destroyed, so we can ignore the last (partially constructed) sub-array.
+ if constexpr (!is_array_v<_ValueType>)
+ ++__i;
+ __destroy_elements(__first, __i);
----------------
The way I understand this is that we should actually always call `__destroy_elements(__first, __i + 1);` to destroy the half-open range `[__first, __first + __i + 1)` (which includes the last object that was fully constructed, but not the object that was partially constructed). However, when the current element (`__first + __i`) is an array, it has already been destroyed before we caught the exception, so we can skip it and just destroy `[__first, __first + __i)`.
This implementation makes that intent clearest IMO:
```
// <explanation>
if constexpr (is_array_v<_ValueType>)
__destroy_elements(__first, __i);
else
__destroy_elements(__first, __i + 1);
```
Do you agree?
================
Comment at: libcxx/include/memory:2775
+
+ [[no_unique_address]] _Alloc __alloc;
+ size_t __size;
----------------
We generally use a trailing underscore (like `__alloc_`) to make it clear it's a member.
================
Comment at: libcxx/include/memory:2716
+template<class _Tp, class = _EnableIf<!is_array_v<_Tp>>>
+void __shared_ptr_destroy_elem(_Tp* __ptr) { __ptr->~_Tp(); }
+
----------------
zoecarver wrote:
> 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.
Well, one place where we might want to use it is in the statically-sized array version of the block (see below why I think we should have one).
================
Comment at: libcxx/include/memory:3498
-template<class _Tp, class ..._Args, class = _EnableIf<!is_array<_Tp>::value> >
+#if _LIBCPP_STD_VER > 17
+
----------------
zoecarver wrote:
> 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.
> 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 don't think that works, actually. For example, `std::make_shared<int[3]>(1, 2, 3)` should SFINAE away, but with your implementation that would be a hard error. Can you add a test and see whether that's correct? If so, it means we need to conform to the declarations laid out in the standard (there's often reasons why the standard lists overloads explicitly).
================
Comment at: libcxx/include/memory:3526
+{
+ return __allocate_shared_array<_Tp>(__a, extent_v<_Tp>, __u);
+}
----------------
zoecarver wrote:
> 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.
We save on storing the size itself in the control block, and the size is known statically, which could lead to different code being generated (e.g for small arrays). I think it's fairly important to do it, since it isn't very costly for us.
The representation for statically-sized arrays can be:
```
[[no_unique_address]] _Alloc __alloc_;
union { // make sure the default constructor doesn't initialize __data_
_Tp __data_[_Np];
};
```
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