[libcxx-commits] [PATCH] D62760: allocate_shared should call allocator_traits::construct

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/memory:3506
+        _VSTD::alignment_of<_CntrlBlk>::value
+    >::type __cntrl_buff[sizeof(_CntrlBlk)];
+
----------------
Wait, so you have an array of `sizeof(ControlBlock)` elements of aligned storage with `sizeof(ControlBlock)`? That's `sizeof(ControlBlock) * sizeof(ControlBlock)` bytes of storage. Why do you need that?


================
Comment at: libcxx/include/memory:3511
+        _VSTD::alignment_of<_CntrlBlk>::value
+    >::type __value_buff[sizeof(_Tp)];
+};
----------------
Similar comment but with `sizeof(_Tp) * sizeof(_Tp)`. I also don't understand why this needs to be aligned to `alignment_of<ControlBlock>`. Can you explain?


================
Comment at: libcxx/include/memory:3611
     __compressed_pair<__compressed_pair<_Tp, _Dp>, _Alloc> __data_;
+    size_t __size;
+
----------------
Isn't this an ABI break? It's not clear to me why we need to store the size -- we should always be calling `allocator.deallocate` with one instance of `sizeof(__shared_ptr_pointer)`, no?


================
Comment at: libcxx/include/memory:4546
+    typedef allocator_traits<_TAlloc> _ATraits;
+    typedef __allocator_destroyer<_TAlloc> _D1;
+
----------------
These variable names (like `_D1`) are not really helpful. I know there weren't better before, but it would be easier to read with a better name.


================
Comment at: libcxx/include/memory:4548
+
+    typedef __shared_ptr_pointer<_Tp*, _D1, _Alloc> _CntrlBlk;
+    typedef __shared_ptr_aligned_block<_CntrlBlk, _Tp> _AlignedBlk;
----------------
Shouldn't this be `<_Tp*, _D1, _TAlloc>` instead of `<_Tp*, _D1, _Alloc>`? If not, why?


================
Comment at: libcxx/include/memory:4564
+    __hold2.release();
+    return shared_ptr<_Tp>::__create_with_control_block(__value_buff, __cntrl_buff);
 }
----------------
`__create_with_control_block` needs to be marked as `noexcept` if you want to do this and make it clear that it's safe. Otherwise we could leak `__hold2` in case an exception was thrown from within `__create_with_control_block`.


================
Comment at: libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp:41
   {
+    // expected-error at memory:* {{no member named 'value' in 'std::__1::is_empty<std::__1::__compressed_pair<void (*)(Tag<4>), std::__1::default_delete<void (Tag<4>)> > >'}}
+    // expected-error at memory:* {{cannot initialize return object of type 'std::__1::__shared_weak_count *' with an rvalue of type 'std::__1::__shared_ptr_pointer<void (*)(Tag<4>), std::__1::default_delete<void (Tag<4>)>, std::__1::allocator<std::__1::__shared_ptr_dummy_rebind_allocator_type> > *'}}
----------------
This `expected-error` is brittle, it won't work if we e.g. change the ABI namespace.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Could you add a test (in another test file) with the reproducer for https://bugs.llvm.org/show_bug.cgi?id=41900, with both private and protected constructors? IIUC, the intent of this patch was primarily to fix that.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp:16
+
+// This patch tests that allocator_traits::construct is used in allocate_shared.
+
----------------
Please explain why this needs to be tested in the comment. It could be as simple as "[...] as requested in C++20", which is the case I think.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp:109
+{
+    // TODO: in C++03 mode construct only works for some variadics.
+#if TEST_STD_VER >= 11
----------------
Please fix this `todo`. It's fine to assume that C++03 has variadics, cause we only support C++03 with Clang, which supports variadics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62760