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

[David Benjamin via Phabricator via libcxx-commits]

davidben added inline comments.


================
Comment at: libcxx/include/optional:641
 template <class _Tp>
-class optional
+class _LIBCPP_DECLSPEC_EMPTY_BASES optional
     : private __optional_move_assign_base<_Tp>
----------------
Mordante wrote:
> 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.
Updated the commit message.

Couldn't find a good place to work it into index.rst, as it doesn't talk about ABI much in the first place, but I added a paragraph to ABIVersioning.rst. This what you had in mind?


================
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>), "");
----------------
Mordante wrote:
> This is not needed in C++17, the same for the static_asserts below.
Done. I've left the existing instances in the file alone.


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

https://reviews.llvm.org/D146190