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

Let me start by saying that I haven't spent a lot of time looking at it but I really like this implementation.

---

In D91201#2453425 <https://reviews.llvm.org/D91201#2453425>, @ldionne wrote:

> Another approach, which Zoe thought about just now and told me on Slack, would be to implement the storage like this instead (untested, I just wrote this up):
>
>   struct _Storage {
>       _LIBCPP_HIDE_FROM_ABI explicit _Storage(_Alloc&& __a) {
>           ::new ((void*)__get_alloc()) _Alloc(_VSTD::move(__a));
>       }
>       _LIBCPP_HIDE_FROM_ABI ~_Storage() {
>           __get_alloc()->~_Alloc();
>       }
>       _Alloc* __get_alloc() _NOEXCEPT { /* compute offset */ }
>       _Tp* __get_elem() _NOEXCEPT { /* compute offset */ }
>       char alignas(__compressed_pair<_Alloc, _Tp>) __blob_[sizeof(__compressed_pair<_Alloc, _Tp>)];
>   };
>
> I think we'd be able to compute the offsets with a bit of help from the `__compressed_pair`, and then I'm pretty sure it would be valid to placement-new into the blob as we like. I'll explore this approach too to see what it gives - the benefit would be not needing `[[no_unique_address]]` and potentially simpler code.

If the approach I'm talking about is simpler in this patch, it might not be simpler in absolute terms. If we're going to implement `_Storage` anyway as a more general thing, I think this implementation makes a lot of sense.

I think the code snippet I quoted is kind of missing my point. Using aligned storage, or a union, or anything similar is a bit irrelevant to what I was trying to say. I was highlighting a good way to do the "compute offset" part. The fundamental problem that this patch is trying to solve is that we need a pointer to the second element of `__data_` in `__shared_ptr_emplace`, and we need that pointer before `__shared_ptr_emplace` is initialized. One way to do that (which you implement in this patch) is to make the second member a union and take its address. Another solution (what I was proposing) is we could simply static cast a pointer to `__data_` to the correct `__compressed_pair_elem` type. Because `__compressed_pair` inherits from both of its elements, this should Just Work™. The latter solution would require no changes to the layout of `__shared_ptr_emplace` (and depending on how you look at it, that might be positive).



================
Comment at: libcxx/include/memory:2602
+    explicit __shared_ptr_emplace(_Alloc __a, _Args&& ...__args)
+        : __storage_{_VSTD::move(__a)}
+    {
----------------
This reminds me that we should support/test move-only allocator types. 


================
Comment at: libcxx/include/memory:2625
+        using _ControlBlockPointer = typename allocator_traits<_ControlBlockAlloc>::pointer;
+        _ControlBlockAlloc __tmp(*__get_alloc());
+        __storage_.~_Storage();
----------------
Do we really need `__tmp` allocators everywhere? 


================
Comment at: libcxx/include/memory:2630
+    }
+
+    // This implements an ABI compatible emulation of libc++'s compressed pair
----------------
IMHO we should make `_Storage` a more general utility.

Also can you please static assert that the alignment and size is the same as an equivilant `__compressed_pair`.


================
Comment at: libcxx/include/memory:2634
+    // initialized when the "compressed pair" is initialized.
+    template <class _AAlloc, class _TTp, bool = __libcpp_is_final<_AAlloc>::value, bool = __libcpp_is_final<_TTp>::value>
+    struct _Storage;
----------------
This class is a work of art :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91201