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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 8 inline comments as done.
zoecarver added a comment.

Thanks for the review!



================
Comment at: libcxx/include/memory:3506
+        _VSTD::alignment_of<_CntrlBlk>::value
+    >::type __cntrl_buff[sizeof(_CntrlBlk)];
+
----------------
ldionne wrote:
> 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?
🤦‍♂️ Oops. Good catch. I originally had `alignas(_CntrlBlk) char __cntrl_buff[sizeof(_CntrlBlk)]` which didn't work in C++03 mode so, I used `aligned_storage` and seem to have forgotten to remove the array part. 


================
Comment at: libcxx/include/memory:3611
     __compressed_pair<__compressed_pair<_Tp, _Dp>, _Alloc> __data_;
+    size_t __size;
+
----------------
ldionne wrote:
> 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?
I was worried this might be an ABI break. 

We need size, though. Because we need to know how big `__shared_ptr_aligned_block` is (and later we'll need to know how big `__shared_ptr_aligned_block` + elements is). 

`sizeof(__shared_ptr_pointer)` would just be the control block. If we knew that `__shared_ptr_pointer` would only be used by `allocate_shared` we could do something like `sizeof(__shared_ptr_aligned_block< __shared_ptr_pointer , remove_pointer_t<_Tp>>)`. To do that we'd probably have to duplicate `__shared_ptr_pointer` which would be an ABI break too, I think.


================
Comment at: libcxx/include/memory:4546
+    typedef allocator_traits<_TAlloc> _ATraits;
+    typedef __allocator_destroyer<_TAlloc> _D1;
+
----------------
ldionne wrote:
> 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.
You're right. I'll make these more clear. 


================
Comment at: libcxx/include/memory:4548
+
+    typedef __shared_ptr_pointer<_Tp*, _D1, _Alloc> _CntrlBlk;
+    typedef __shared_ptr_aligned_block<_CntrlBlk, _Tp> _AlignedBlk;
----------------
ldionne wrote:
> Shouldn't this be `<_Tp*, _D1, _TAlloc>` instead of `<_Tp*, _D1, _Alloc>`? If not, why?
It doesn't really matter because it gets rebound to `char` before being called. I'll put a comment here.


================
Comment at: libcxx/include/memory:4564
+    __hold2.release();
+    return shared_ptr<_Tp>::__create_with_control_block(__value_buff, __cntrl_buff);
 }
----------------
ldionne wrote:
> `__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`.
OK, I'll make that a separate patch, though. `__create_with_control_block` was already used here.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> 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.
Yes. I'll add a test for that. Good call. 


================
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.
+
----------------
ldionne wrote:
> 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.
Huh, you're right. I didn't realize this was since C++20. I'll add the comment. 


================
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
----------------
ldionne wrote:
> 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.
Yes, I'll fix this. I want to make it a separate patch because it will be a fairly large change. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62760