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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 6 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/memory:2720
+_LIBCPP_HIDE_FROM_ABI
+void __shared_ptr_array_construct_elements(_Alloc const& __alloc, _ValueType *__first, size_t __n, _Args&& ...__args) {
+    using _ValueAlloc = typename __allocator_traits_rebind<_Alloc, _ValueType>::type;
----------------
Why not pass by value?


================
Comment at: libcxx/include/memory:2752
+        else
+            __shared_ptr_array_destroy_elements(__alloc, __first, __i + 1);
+        throw;
----------------
ldionne wrote:
> I haven't looked for that specifically, but can you confirm that you are testing these two code paths?
Yes, these two code paths are tested. See L324 where we use `struct D` to throw. 


================
Comment at: libcxx/include/memory:2787
+        _CharAlloc __tmp(__alloc_);
+        allocator_traits<_CharAlloc>::deallocate(__tmp, reinterpret_cast<char*>(this), __get_size());
+    }
----------------
ldionne wrote:
> Don't we need to run the destructor of `__alloc_` here?
Yep. Added a regression test, too. 


================
Comment at: libcxx/include/memory:2793
+    using _DataBlock = typename std::aligned_storage<sizeof(_Tp), alignof(_Tp)>::type;
+    _DataBlock __data_[0];
+};
----------------
ldionne wrote:
> Is this legal C++? I thought that was a compiler extension and arrays had to have at least one element?
Yes, you're right. Fixed. (Supprising that we didn't get a warning, though?)


================
Comment at: libcxx/include/memory:2803
+    _LIBCPP_HIDE_FROM_ABI
+    size_t __get_size() noexcept { return sizeof(__shared_ptr_array_bound); }
+
----------------
ldionne wrote:
> 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?
Agreed. Same with `__get_count`.


================
Comment at: libcxx/include/memory:3552
+
+    size_t __size = __n * sizeof(_ElemT) + sizeof(_ControlBlock);
+    __allocation_guard<_CharAllocator> __guard(__a, __size);
----------------
ldionne wrote:
> 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.
Moot because I fixed `__data_[0 -> 1]`. 

However, I thought this was the whole point of using a zero size array at the end of a struct? Otherwise, why not just make an empty struct. So, yes, that padding would be accounted for:
```
struct UnsizedTail {
  int size;
  alignas(8) char buf[0];
};

// UnsizedTail = {i32, [4 x i8 padding], [0 x i8]
static_assert(sizeof(UnsizedTail) == 8);
```

That being said... when would there be padding between `size_t` and an aligned storage member?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62641