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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver reclaimed this revision.
zoecarver marked an inline comment as done.
zoecarver added inline comments.
This revision now requires changes to proceed.


================
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:
> zoecarver wrote:
> > ldionne wrote:
> > > zoecarver wrote:
> > > > ldionne wrote:
> > > > > 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.
> > > > Is `memcpy`ing an object any different than copying a char array of its data? I'
> > > > ll give it a try in a constexpr context. 
> > > No, they are the same.
> > > 
> > > But my point is that it's not a valid way to copy a *non-trivially-copyable* object. That's the whole point of being non trivially copyable. Does that make sense?
> > Hmm, I see what you're saying. For what it's worth, up until last week, we would use and copy an array of chars. But, just because we did something wrong for a long time doesn't mean we should continue. I'll close this revision, then. We'll see what LWG says. 
> Can you try it out in a constexpr context with the approach where we memcpy an array of `char`? Clang must detect UB inside constexpr, so I'm pretty sure it would tell you (and hence that wouldn't be a viable constexpr implementation).
`memcpy`ing the non-active union member address works in a constexpr. BUT we cannot assign or read it (in a constexpr only, we can still assign and read it outside of a constexpr context). This seems OK to me given the alternative is not being able to read or assign it in any context (and having the error be a runtime, not constexpr one).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81174