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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__memory/compressed_pair.h:164
+
+template <class _T1, class _T2>
+class __compressed_pair {
----------------
ldionne wrote:
> I would like to better understand the benefit of making this change. What do we gain? At a glance, we seem to gain EBO for types marked `final` -- I think that's it, right?
> 
> I wonder whether it's worth adding complexity for that, but I'm not closed to the idea.
There are a few things:
(1) `final` types are compressed
(2) slightly faster compile times, because less templates have to be instantiated (I haven't measured)
(3) this change has to be made to allow using `[[no_unique_address]]` directly in the classes if the support for ABIv1 will ever be dropped


================
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>;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122135