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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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
+{
----------------
Quuxplusone wrote:
> 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.
I like the fact that we can confirm by inspection that the class is being instantiated on `_Tp[]` and not `_Tp[N]`. If I made it the base template, I would probably want to add a `static_assert`, but then I might as well use the status quo, which is simpler. I'm inclined not to change anything.


================
Comment at: libcxx/include/__memory/shared_ptr.h:988
+    {
+        _VSTD::__uninitialized_allocator_fill_n(__alloc_, _VSTD::begin(__data_), __count_, __arg);
+    }
----------------
Quuxplusone wrote:
> `__data_` is an array.
I know, but given that we're working with multidimensional arrays, it can become extremely confusing and I think the `std::begin` adds clarity, so I'm tempted to keep it.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1068
+    _LIBCPP_HIDE_FROM_ABI
+    _Tp* __get_elem_begin() noexcept { return reinterpret_cast<_Tp*>(&__data_); }
+
----------------
Quuxplusone wrote:
> 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.
I don't think this specific one is an ADL issue, because we're taking the address of the array, not the address of the first element (see https://godbolt.org/z/KMjezb6qE). But I'll still simplify this and stop taking the address, which is indeed not necessary. It *is* an issue in places below where we do `&__data_[0]` and I fixed it there and added tests.

I can't get rid of the method entirely without making `__data_` public, which I'd rather not do, so I'll keep it around.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1080-1081
+
+    _LIBCPP_HIDE_FROM_ABI
+    ~__bounded_array_control_block() { }
+
----------------
Quuxplusone wrote:
> This should be `=default`...
It turns out this doesn't work because `__data_` is sometimes non-trivial, and a non-trivial member in a union results in a the destructor being deleted (which makes sense, because you might need to check which of the non-trivial members of the union needs to be deleted). I added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62641