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

[ldionne]

Summary

Brings back the "union trick" so that we can stop returning nullptr from data and range methods. This patch adds two overloads of the union __array_storage_wrapper. One for trivially copyable types, and one for non-trivially copyable types (where we define a copy constructor).

[miscco]

I am still having trouble to unterstand what benefit this has.

[zoecarver]

I am still having trouble to unterstand what benefit this has.

The benefit is two-fold. First, it fixes the ABI break. Second, this seems like the preferred behavior, if possible. IMO it's the most intuitive implementation. After the LWG issue around array<T, 0> goes through, I suspect the wording will be clarified to make the result of data() != nullptr. And, for people who use array<T, 0> (if there are any) it will be nice not to have to update their code to expect data() to return nullptr and then again to expect data() not to return nullptr.

[ldionne]

libcxx/include/array

238

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?

250

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?

libcxx/test/std/containers/sequences/array/iterators.pass.cpp

100

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?

[zoecarver]

libcxx/include/array

238

The union is trivially copyable when it's members are. So, yes, it will be trivially copyable. And the test does pass :)

250

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.

libcxx/test/std/containers/sequences/array/iterators.pass.cpp

100

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).

[ldionne]

libcxx/include/array

250

I mean, even if you use memcpy, it's UB. You can't summon a non trivially copyable object into existence merely by memcpying 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.

libcxx/test/std/containers/sequences/array/iterators.pass.cpp

100

Please only put one in initialization.pass.cpp.

[zoecarver]

libcxx/include/array

250

Is memcpying an object any different than copying a char array of its data? I'
ll give it a try in a constexpr context.

[ldionne]

libcxx/include/array

250

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?

[zoecarver]

libcxx/include/array

250

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.

[ldionne]

libcxx/include/array

250

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).

[zoecarver]

libcxx/include/array

250

memcpying 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).

[ldionne]

Is there still interest in exploring this? If not, let's close to clear up the queue.

[ldionne]

[Github PR transition cleanup]

Abandoning to clear the queue.

libcxx/include/array

struct _LIBCPP_TEMPLATE_VIS array
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11 const_reference back() const  _NOEXCEPT {return (*this)[_Size - 1];}
 
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     value_type* data() _NOEXCEPT {return __elems_;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     const value_type* data() const _NOEXCEPT {return __elems_;}
 };
 
+template<class _Tp, bool = is_trivially_copyable<_Tp>::value>
+union __array_storage_wrapper;
+
+template<class _Tp>
+union __array_storage_wrapper<_Tp, true> {
+    _LIBCPP_CONSTEXPR __array_storage_wrapper() : __b() { }
+    ~__array_storage_wrapper() = default;
+
+    bool __b;
+    _Tp  __t;
+};
+
+template<class _Tp>
+union __array_storage_wrapper<_Tp, false> {
+    _LIBCPP_CONSTEXPR __array_storage_wrapper() : __b() { }
+    ~__array_storage_wrapper() = default;
+    _LIBCPP_CONSTEXPR __array_storage_wrapper(__array_storage_wrapper const& __other)
+        : __t(__other.__t) { }
+    _LIBCPP_CONSTEXPR_AFTER_CXX11 __array_storage_wrapper& operator=(__array_storage_wrapper const& __other)
+        { __t = __other.__t; return *this; }
+
+    bool __b;
+    _Tp  __t;
+};
+
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS array<_Tp, 0>
 {
     // types:
     typedef array __self;
     typedef _Tp                                   value_type;
     typedef value_type&                           reference;
     typedef const value_type&                     const_reference;
     typedef value_type*                           iterator;
     typedef const value_type*                     const_iterator;
     typedef value_type*                           pointer;
     typedef const value_type*                     const_pointer;
     typedef size_t                                size_type;
     typedef ptrdiff_t                             difference_type;
     typedef std::reverse_iterator<iterator>       reverse_iterator;
     typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
 
     typedef typename conditional<is_const<_Tp>::value, const char,
                                 char>::type _CharType;
 
+#if _LIBCPP_STD_VER > 14
+    __array_storage_wrapper<_Tp> __w;
+#else
     struct  _ArrayInStructT { _Tp __data_[1]; };
     _ALIGNAS_TYPE(_ArrayInStructT) _CharType __elems_[sizeof(_ArrayInStructT)];
+#endif // _LIBCPP_STD_VER > 14
 
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    value_type* data() _NOEXCEPT {return nullptr;}
+    value_type* data() _NOEXCEPT
+    {
+#if _LIBCPP_STD_VER > 14
+        return &__w.__t;
+#else
+        return reinterpret_cast<value_type*>(__elems_);
+#endif // _LIBCPP_STD_VER > 14
+    }
+
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    const value_type* data() const _NOEXCEPT {return nullptr;}
+    const value_type* data() const _NOEXCEPT
+    {
+#if _LIBCPP_STD_VER > 14
+        return &__w.__t;
+#else
+        return reinterpret_cast<const value_type*>(__elems_);
+#endif // _LIBCPP_STD_VER > 14
+    }
 
     // No explicit construct/copy/destroy for aggregate type
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
     void fill(const value_type&) {
       static_assert(!is_const<_Tp>::value,
                     "cannot fill zero-sized array of type 'const T'");
     }
 

libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp

TEST_CONSTEXPR_CXX17 bool tests()
         assert(p[1] == 2);
         assert(p[2] == 3.5);
     }
     {
         typedef double T;
         typedef std::array<T, 0> C;
         C c = {};
         T* p = c.data();
-        (void)p;
+        assert(p != nullptr);
     }
     {
         typedef double T;
         typedef std::array<const T, 0> C;
         C c = {{}};
         const T* p = c.data();
-        (void)p;
+        assert(p != nullptr);
         static_assert((std::is_same<decltype(c.data()), const T*>::value), "");
     }
     {
         typedef NoDefault T;
         typedef std::array<T, 0> C;
         C c = {};
         T* p = c.data();
-        (void)p;
+        assert(p != nullptr);
     }
     {
         std::array<int, 5> c = {0, 1, 2, 3, 4};
         assert(c.data() == &c[0]);
         assert(*c.data() == c[0]);
     }
 
     return true;

 #if TEST_STD_VER < 11
         typedef natural_alignment T;
 #else
         typedef std::max_align_t T;
 #endif
         typedef std::array<T, 0> C;
         const C c = {};
         const T* p = c.data();
+        assert(p != nullptr);
         std::uintptr_t pint = reinterpret_cast<std::uintptr_t>(p);
         assert(pint % TEST_ALIGNOF(T) == 0);
     }
     return 0;
 }

libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp

TEST_CONSTEXPR_CXX17 bool tests()
         assert(p[1] == 2);
         assert(p[2] == 3.5);
     }
     {
         typedef double T;
         typedef std::array<T, 0> C;
         const C c = {};
         const T* p = c.data();
-        (void)p;
+        assert(p != nullptr);
     }
     {
         typedef NoDefault T;
         typedef std::array<T, 0> C;
         const C c = {};
         const T* p = c.data();
-        (void)p;
+        assert(p != nullptr);
     }
     {
         std::array<int, 5> const c = {0, 1, 2, 3, 4};
         assert(c.data() == &c[0]);
         assert(*c.data() == c[0]);
     }
 
     return true;

 #if TEST_STD_VER < 11
         typedef natural_alignment T;
 #else
         typedef std::max_align_t T;
 #endif
         typedef std::array<T, 0> C;
         const C c = {};
         const T* p = c.data();
+        assert(p != nullptr);
         std::uintptr_t pint = reinterpret_cast<std::uintptr_t>(p);
         assert(pint % TEST_ALIGNOF(T) == 0);
     }
 
     return 0;
 }

libcxx/test/std/containers/sequences/array/iterators.pass.cpp

TEST_CONSTEXPR_CXX17 bool tests()
         assert(i == j);
     }
     {
         typedef std::array<int, 0> C;
         C array = {};
         typename C::iterator i = array.begin();
         typename C::const_iterator j = array.cbegin();
         assert(i == j);
+        assert(i != nullptr);
+        assert(j != nullptr);
     }
 
     {
         typedef std::array<int, 0> C;
         C array = {};
         typename C::iterator i = array.begin();
         typename C::const_iterator j = array.cbegin();
         assert(i == array.end());
         assert(j == array.cend());
+        assert(i != nullptr);
+        assert(j != nullptr);
     }
     {
         typedef std::array<int, 1> C;
         C array = {1};
         typename C::iterator i = array.begin();
         assert(*i == 1);
         assert(&*i == array.data());
         *i = 99;
TEST_CONSTEXPR_CXX17 bool tests()
         assert(*i == 1);
         assert(&*i == array.data());
         *i = 5.5;
         assert(array[0] == 5.5);
         assert(array[1] == 2.0);
     }
     {
         typedef std::array<NoDefault, 0> C;
+        static_assert(std::is_trivially_copyable<NoDefault>::value, "");
         C array = {};
         typename C::iterator ib = array.begin();
         typename C::iterator ie = array.end();
         assert(ib == ie);
+        assert(ib != nullptr);
+        assert(ie != nullptr);
     }
 
 #if TEST_STD_VER >= 14
     { // N3644 testing
         {
             typedef std::array<int, 5> C;
             C::iterator ii1{}, ii2{};
             C::iterator ii4 = ii1;