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

[Louis Dionne via Phabricator via libcxx-commits]

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