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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__memory/shared_ptr.h:975-980
+template <class _Tp, class _Alloc>
+struct __unbounded_array_control_block;
+
+template <class _Tp, class _Alloc>
+struct __unbounded_array_control_block<_Tp[], _Alloc> : __shared_weak_count
+{
----------------
IMO it would be cleaner to eliminate the partial specialization and just write
```
template <class _Array, class _Alloc>
struct __unbounded_array_control_block : __shared_weak_count
{
    using _Tp = remove_extent_t<_Array>;
```
Ditto for `__bounded_array_control_block` below. But admittedly I'd expect this to be marginally more expensive in terms of compile time.


================
Comment at: libcxx/include/__memory/shared_ptr.h:988
+    {
+        _VSTD::__uninitialized_allocator_fill_n(__alloc_, _VSTD::begin(__data_), __count_, __arg);
+    }
----------------
`__data_` is an array.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1007-1008
+        // [1]: https://en.wikipedia.org/wiki/Data_structure_alignment#Computing_padding
+        size_t __bytes = __elements <= 1 ? sizeof(__unbounded_array_control_block)
+                                         : (__elements - 1) * sizeof(_Tp) + sizeof(__unbounded_array_control_block);
+        constexpr size_t __align = alignof(_Tp);
----------------
The condition could be `__elements == 0 ? ~~~`; the only singularity requiring a special case is at zero. It looks to me as if `make_shared<int[]>(0)` is intended to be UB: https://eel.is/c++draft/util.smartptr.shared#create-13 — but maybe we want to support it anyway for QoI, or maybe I'm misunderstanding the possibility of returning a pointer to an object of type `U[0]`.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1068
+    _LIBCPP_HIDE_FROM_ABI
+    _Tp* __get_elem_begin() noexcept { return reinterpret_cast<_Tp*>(&__data_); }
+
----------------
This isn't ADL-proof — add a regression test!
And then this can just be `return __data_;` because `__data_` is an array, and then this function doesn't need to exist.
Ditto lines 1072, 1077, maybe elsewhere.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1080-1081
+
+    _LIBCPP_HIDE_FROM_ABI
+    ~__bounded_array_control_block() { }
+
----------------
This should be `=default`...


================
Comment at: libcxx/include/__memory/shared_ptr.h:1084
+private:
+    virtual void __on_zero_shared() _NOEXCEPT {
+        __allocator_traits_rebind_t<_Alloc, _Tp> __value_alloc(__alloc_);
----------------
...and this should be `override` (not `virtual`), IIUC.
(Ditto throughout.)


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:358-360
+template<class _Alloc, class _BidirIter, class = __enable_if_t<
+    __is_cpp17_bidirectional_iterator<_BidirIter>::value
+>>
----------------
Why does this need to work with non-pointers at all? I'm 99% positive it should take `_Tp*` instead of an arbitrary `_BidirIter`. Likewise throughout — there are a lot of these unnecessarily bidirectional-iterator helpers.
Likewise line 363: `using _ValueType = _Tp` and then there's no more need for `_ValueType` to exist.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62641/new/

https://reviews.llvm.org/D62641