[libcxx-commits] [PATCH] D146190: Fix EBO on std::optional and std::variant when targeting the MSVC ABI

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Since @EricWF feels this is fine for Windows I don't have objections either. However I really feel we need to document this better.



================
Comment at: libcxx/include/optional:641
 template <class _Tp>
-class optional
+class _LIBCPP_DECLSPEC_EMPTY_BASES optional
     : private __optional_move_assign_base<_Tp>
----------------
davidben wrote:
> EricWF wrote:
> > Mordante wrote:
> > > Why not guard this attribute with a `_LIBCPP_ABI_FOO_FLAG` instead of breaking the ABI unconditionally?
> > Because that would prevent most users from ever getting value from it.
> > 
> > If we don't have any libc++ users who care about ABI compatibility with MSVC, then we should do this unconditionally.
> Unconditional is also my preference, but I'll do whatever the project prefers. Currently assuming that's to leave it at is. (What's the process for driving that question to a conclusion?)
When that is the rationale for forcing an ABI break on all Windows users, then let's document that in the commit message.

I think it would be good to add this rationale to the `index.rst`, since this is against the normal libc++ policy. I really want to avoid people thinking an unstable ABI is acceptable on all platforms.


================
Comment at: libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp:81
+  // Test that std::variant achieves the expected size. See https://github.com/llvm/llvm-project/issues/61095.
+  static_assert(sizeof(std::variant<char, char, char>) == sizeof(type_with_index<char>), "");
+  static_assert(sizeof(std::variant<int, int, int>) == sizeof(type_with_index<int>), "");
----------------
This is not needed in C++17, the same for the static_asserts below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146190