[libcxx-commits] [PATCH] D81174: [libcxx] Return "real" pointer from array<T, 0>::data.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/array:238
+template<class _Tp>
+union __array_storage_wrapper<_Tp, true> {
+    _LIBCPP_CONSTEXPR __array_storage_wrapper() : __b() { }
----------------
I think the problem we had remains -- it's that the union itself isn't trivially copyable even if `_Tp` is, no? Does `libcxx/test/libcxx/containers/sequences/array/triviality.pass.cpp` pass with this implementation?


================
Comment at: libcxx/include/array:250
+    ~__array_storage_wrapper() = default;
+    _LIBCPP_CONSTEXPR __array_storage_wrapper(__array_storage_wrapper const& __other)
+        : __t(__other.__t) { }
----------------
I think this is UB: we're only ever constructing the `__b` member, yet the copy-constructor copies the `__t` member, which hasn't been initialized. I think this triggers UB when we copy an empty array, which ends up calling this copy constructor on a union member that hasn't been initialized yet.

WDYT?


================
Comment at: libcxx/test/std/containers/sequences/array/iterators.pass.cpp:100
         typedef std::array<NoDefault, 0> C;
+        static_assert(std::is_trivially_copyable<NoDefault>::value, "");
         C array = {};
----------------
This is a great addition, but it doesn't strike me as the right place to put this assertion. Instead, shouldn't that be in libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81174