[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/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp:24
+int main(int, char**) {
+  // Test that std::optional achieves the expected size. See https://github.com/llvm/llvm-project/issues/61095.
+  static_assert(sizeof(std::optional<char>) == sizeof(type_with_bool<char>));
----------------
ldionne wrote:
> 
Done.


================
Comment at: libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp:67
+#ifdef _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+  unsigned char index;
+#else
----------------
ldionne wrote:
> Can't we always use `unsigned char`? It would be nice to avoid checking an internal macro like `_LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION` from the test suite if we can.
I don't think that works. This is trying to model the expected size of `std::variant<T>` but, when `_LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION` is unset, the index is `unsigned int` instead of `unsigned char`. You all, I gather, couldn't apply the optimization unconditionally due to ABI stability.

This isn't the only place in the file that needs to handle this. See `ExpectEqual` a few lines up in the file.


================
Comment at: libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp:80
 
+  // 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>));
----------------
ldionne wrote:
> 
Done.


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

https://reviews.llvm.org/D146190