[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.

This basically LGTM except for the few nitpicks below. We already talked about most of that offline, just adding here so you have a written trace.



================
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;
----------------
zoecarver wrote:
> zoecarver wrote:
> > Why not pass by value?
> Whoops. I thought I deleted this comment. This was just a note to myself. 
> 
> Anyway, I am actually wondering about this. I see `_Alloc const&` all over the place, but allocators are generally supposed to be small (often empty) types, so why not pass them by value? We're copy constructing the allocator in this function, so it can't be to prevent that.
Generally, we take by value (and then move into some internal storage) when we take ownership of an object -- or we add a version that takes that object by rvalue reference. You could do that here and move into `__value_alloc` if you wanted.


================
Comment at: libcxx/include/memory:3536
+_LIBCPP_HIDE_FROM_ABI
+shared_ptr<_Tp> __allocate_shared_array_unbound(const _Alloc& __a, size_t __n, _Args&& ...__args)
+{
----------------
We need to handle the case where `__n == 0`. Also, we should add a test for it since I'm not seeing anything that says it's not allowed by the API.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/create_array.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Can we add a test with overaligned types? (like we discussed offline)


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/create_array.pass.cpp:9
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
----------------
You can remove `c++98` from the Lit features, we don't use it anymore.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/create_array.pass.cpp:47
+  A(const A&) : value(++count) {}
+  ~A() { assert(count-- == value); }
+};
----------------
zoecarver wrote:
> This should help make sure we destroy things in the correct order. 
Can you add a comment mentioning that? It's clever.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62641