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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/memory:2625
+        using _ControlBlockPointer = typename allocator_traits<_ControlBlockAlloc>::pointer;
+        _ControlBlockAlloc __tmp(*__get_alloc());
+        __storage_.~_Storage();
----------------
zoecarver wrote:
> Do we really need `__tmp` allocators everywhere? 
We need this one because we destroy the original one when we call `~Storage()`, and then we need to deallocate `this` using the allocator. So I really don't see a way to get around this.


================
Comment at: libcxx/include/memory:2630
+    }
+
+    // This implements an ABI compatible emulation of libc++'s compressed pair
----------------
zoecarver wrote:
> 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`.
> IMHO we should make `_Storage` a more general utility.

I think I agree, but I'd like to do that in a separate patch. Making it generic will add complexity, like the ability to skip initialization of either member, etc. I'd like to keep this change small because of the trickiness.

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

Done.


================
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;
----------------
zoecarver wrote:
> This class is a work of art :)
I'm not sure whether I want to say thank you or run away and hide :-). I guess the problem we're trying to solve is pretty terrible, but the solution isn't too bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91201