[libcxx-commits] [PATCH] D91201: [libc++] LWG2070: Use Allocator construction for objects created with allocate_shared

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

This LGTM. Not only to fix the LWG issue, but this is a great cleanup to this part of the codebase. So, thanks Louis.



================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp:38
+struct OldEmplaceControlBlock
+  : std::__shared_weak_count
+{
----------------
Maybe this should be a `libcxx` test only? I see "libcxx" in the name, but maybe it should be moved to the "libcxx" test directory. 


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp:137
+
+int main(int, char**) {
+  test<TrivialEmptyType, TrivialEmptyAlloc>();
----------------
This test makes me feel really confident that there won't be any ABI issues. Thanks!


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp:28
+static bool destroy_called = false;
+static unsigned allocator_id = 0;
+
----------------
ldionne wrote:
> zoecarver wrote:
> > In either patch, it would probably be a good idea to move these into `MyAllocator` so we can make sure that the correct specialization is setting them. 
> I don't think we can, because the allocator is going to be rebound to allocate an unknown object (ie the shared pointer control block).
Fair enough. Makes sense. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91201