[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
Tue Feb 9 16:09:25 PST 2021
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Thanks a lot for all your work on this patch. This is a tough one to get right. I know I'm requesting a lot, but I think it's important to get it right and we'll be even happier when we finally ship this :-).
================
Comment at: libcxx/include/memory:2730
+ if constexpr (is_array_v<_ValueType>) {
+ __shared_ptr_array_construct_elements(__alloc, _VSTD::begin(*(__first + __i)), extent_v<_ValueType>, __args...);
+ } else {
----------------
We could also use `__first[i]` here, I think it's clearer?
================
Comment at: libcxx/include/memory:2752
+ else
+ __shared_ptr_array_destroy_elements(__alloc, __first, __i + 1);
+ throw;
----------------
I haven't looked for that specifically, but can you confirm that you are testing these two code paths?
================
Comment at: libcxx/include/memory:2759
+template <class _Tp, class _Alloc>
+struct __shared_ptr_array_unbound : __shared_weak_count
+{
----------------
Can we name them `xx_bounded` and `xx_unbounded` instead? It seems to fit better?
================
Comment at: libcxx/include/memory:2764-2767
+ _LIBCPP_HIDE_FROM_ABI
+ size_t __get_size() noexcept {
+ return __count_ * sizeof(_Tp) + sizeof(__shared_ptr_array_unbound);
+ }
----------------
Currently, this function is only used in a single place (which is inside the implementation of this class). Furthermore, we duplicate that logic when we compute the size to allocate down there in `make_shared_xx`.
One possibility would be to move this to a free function like
```
template <class T>
size_t __get_shared_ptr_unbounded_block_size(size_t __n) {
// ...
}
```
Then we could reuse it from both places. Do you agree that would be an improvement?
================
Comment at: libcxx/include/memory:2787
+ _CharAlloc __tmp(__alloc_);
+ allocator_traits<_CharAlloc>::deallocate(__tmp, reinterpret_cast<char*>(this), __get_size());
+ }
----------------
Don't we need to run the destructor of `__alloc_` here?
================
Comment at: libcxx/include/memory:2793
+ using _DataBlock = typename std::aligned_storage<sizeof(_Tp), alignof(_Tp)>::type;
+ _DataBlock __data_[0];
+};
----------------
Is this legal C++? I thought that was a compiler extension and arrays had to have at least one element?
================
Comment at: libcxx/include/memory:2803
+ _LIBCPP_HIDE_FROM_ABI
+ size_t __get_size() noexcept { return sizeof(__shared_ptr_array_bound); }
+
----------------
Since this is only used in a single place in the body of `__shared_ptr_array_bound`, I think we could just inline the function at its single point of use and remove it. WDYT?
================
Comment at: libcxx/include/memory:2822
+ using _CharAlloc = typename __allocator_traits_rebind<_Alloc, char>::type;
+ _CharAlloc __tmp(__alloc_);
+ allocator_traits<_CharAlloc>::deallocate(__tmp, reinterpret_cast<char*>(this), __get_size());
----------------
Same comment regarding destruction of `__alloc_`.
================
Comment at: libcxx/include/memory:3552
+
+ size_t __size = __n * sizeof(_ElemT) + sizeof(_ControlBlock);
+ __allocation_guard<_CharAllocator> __guard(__a, __size);
----------------
I *think* this can cause us to under allocate, for example if there needs to be padding between `__count_` and `__data_`:
```
[[no_unique_address]] _Alloc __alloc_;
size_t __count_;
// padding possible here - would that be counted in sizeof(_ControlBlock)?
_DataBlock __data_[0];
```
This concern is moot if I'm right about `_DataBlock __data_[0]` being an extension and if we change to `_DataBlock __data_[1]` instead.
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