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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 3 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/array:238
+template<class _Tp>
+union __array_storage_wrapper<_Tp, true> {
+    _LIBCPP_CONSTEXPR __array_storage_wrapper() : __b() { }
----------------
ldionne wrote:
> 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?
The union is trivially copyable when it's members are. So, yes, it will be trivially copyable. And the test does pass :)


================
Comment at: libcxx/include/array:250
+    ~__array_storage_wrapper() = default;
+    _LIBCPP_CONSTEXPR __array_storage_wrapper(__array_storage_wrapper const& __other)
+        : __t(__other.__t) { }
----------------
ldionne wrote:
> 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?
Yes, I think you're right. `memcpy` is probably the way to go here. I'll use `__builtin_memcpy` so it's still a constexpr on clang. 


================
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 = {};
----------------
ldionne wrote:
> 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?
Oops, this was just for my personal sanity check :P 

It's probably not a bad idea to keep it, though. The struct is defined in this file so, it seems like this file is the right place for it. I'll move it under the struct definition (I can put another one in `initialization.pass.cpp`). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81174