[libcxx-commits] [PATCH] D122135: [libc++] Use [[no_unique_address]] in __compressed_pair in ABIv2

Tue Mar 29 08:18:34 PDT 2022

ldionne added a comment.

I would like us to check whether https://reviews.llvm.org/D63744#2369890 can be implemented (especially since we now support recent compilers only, which wasn't the case back then). If that works, I think this would remove the need for this patch and provide a bunch of additional benefits even in the stable ABI. If not, then I think this patch is fine. Requesting changes for now.

================
Comment at: libcxx/include/__memory/shared_ptr.h:333
     // the same layout that we had when we used a compressed pair.
+#ifndef _LIBCPP_COMPRESSED_PAIR_USE_NO_UNIQUE_ADDRESS
     using _CompressedPair = __compressed_pair<_Alloc, _Tp>;
----------------
philnik wrote:
> ldionne wrote:
> > I don't think this part of the patch should be tied to the change in `__compressed_pair`. It's really not related -- the idea is only that if we're using the unstable ABI, we can suddenly simplify the implementation of the control block, but doing so has nothing to do with using `[[no_unqiue_address]]` inside `__compressed_pair`.
> It kind-of has. The control-block is tied to the layout of the ABIv1 `__compressed_pair`. So the ABIv2 control block has to be used when enabling the ABIv2 `__compressed_pair`. Do you just want to have another macro or should I put in an error if `_LIBCPP_ABI_COMPRESSED_PAIR_USE_NO_UNIQUE_ADDRESS` is set but something like `_LIBCPP_ABI_SHARED_PTR_SIMPLE_CONTROL_BLOCK` is not set? I don't think there are any other `_LIBCPP_ABI*` macros that depend on another yet.
Ahh, I see it now -- there is no `__get_second_base` function after your patch, so the control block implementation does need to change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122135