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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/array:250
+    ~__array_storage_wrapper() = default;
+    _LIBCPP_CONSTEXPR __array_storage_wrapper(__array_storage_wrapper const& __other)
+        : __t(__other.__t) { }
----------------
zoecarver wrote:
> 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. 
I mean, even if you use `memcpy`, it's UB. You can't summon a non trivially copyable object into existence merely by `memcpy`ing it, it's still UB to access the object afterwards IIUC.

You can try, but I would expect that Clang is going to error out if you try to take a pointer to `__t` if it has been "initialized" via `memcpy` -- which in that case means it hasn't been initialized properly -- in a constexpr context.


================
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 = {};
----------------
zoecarver wrote:
> 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`). 
Please only put 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