[libc++] Handle arrays in std::destroy_at

[ldionne]

Summary

Also, improve tests for std::destroy and std::destroy_n so that they
check for array support.

These changes are part of http://wg21.link/p0896 (the One Ranges proposal).

[ldionne]

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy.pass.cpp

127

This is pretty lame, but I couldn't find a way to test array support in constexpr mode before std::construct_at has support for arrays.

The problem is that we run into this kind of issue:

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy.pass.cpp:132:19: error: static_assert expression is not an integral constant expression
    static_assert(test());
                  ^~~~~~
build/default/include/c++/v1/__memory/construct_at.h:40:12: note: construction of subobject of object outside its lifetime is not allowed in a constant expression
    return ::new ((void*)__location) _Tp(_VSTD::forward<_Args>(__args)...);
           ^
build/default/include/c++/v1/__memory/allocator_traits.h:298:9: note: in call to 'construct_at(&{*new Counted [5][3]#2}[0][0], &counter)'
        _VSTD::construct_at(__p, _VSTD::forward<_Args>(__args)...);
        ^

If std::construct_at had support for arrays, we'd create the array using it and that would do the trick. As it is, I'm not sure how to work around it.

[• Quuxplusone]

libcxx/include/__memory/construct_at.h

61

Instead of using this class = void hack, could you just use

template <class _Tp, _EnableIf<!is_array_v<_Tp>, int> = 0>

in the first case and

template <class _Tp, _EnableIf<is_array_v<_Tp>, int> = 0>

in the second case?

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy_n.pass.cpp

131–134

I'd prefer to wrap each offending block in an if:

if (!std::is_constant_evaluated()) {
    using Array = Counted[3];
    ...
}
if (!std::is_constant_evaluated()) {
    using Array = Counted[3][2];
    ...
}

That way if someone adds to the end of the function, it won't get accidentally nerfed.
Alternatively, probably even better: pull the offending blocks out into a separate function, bool test_arrays(), and just don't test it in constexpr mode yet.

[ldionne]

libcxx/include/__memory/construct_at.h

61

Indeed. For some reason I've never liked that trick, but it's not a rational preference. Switched.

[ldionne]

Address review comments

[ldionne]

Fix C++17

[• Quuxplusone]

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy.pass.cpp

92

Could use just constexpr here if you want.

109

FWIW, in my most recent patch I've rekindled my love affair with

ASSERT_SAME_TYPE(decltype(std::destroy(pool, pool + 5)), void);

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy_n.pass.cpp

97–109

Not blocking, but it would be cleaner to factor these repeated blocks into template<class It> void test1() and then have the body of the top-level test() be just test1<Counted*>(); test1<forward_iterator<Counted*>>();
(Applies equally to all the tests touched in this PR, if you choose to do it at all.)

[ldionne]

Apply review feedback (thanks).

libcxx/include/__memory/construct_at.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___MEMORY_CONSTRUCT_AT_H
 #define _LIBCPP___MEMORY_CONSTRUCT_AT_H
 
 #include <__config>
 #include <__debug>
+#include <__iterator/access.h>
+#include <__memory/addressof.h>
 #include <__utility/forward.h>
+#include <type_traits>
 #include <utility>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>

 }
 
 #endif
 
 // destroy_at
 
 #if _LIBCPP_STD_VER > 14
 
-template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+template <class _ForwardIterator>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+void destroy(_ForwardIterator, _ForwardIterator);
+
+template <class _Tp, _EnableIf<!is_array_v<_Tp>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
 void destroy_at(_Tp* __loc) {
     _LIBCPP_ASSERT(__loc, "null pointer given to destroy_at");
     __loc->~_Tp();
 }
 
+#if _LIBCPP_STD_VER > 17
+template <class _Tp, _EnableIf<is_array_v<_Tp>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+void destroy_at(_Tp* __loc) {
+    _LIBCPP_ASSERT(__loc, "null pointer given to destroy_at");
+    _VSTD::destroy(_VSTD::begin(*__loc), _VSTD::end(*__loc));
+}
+#endif
+
+template <class _ForwardIterator>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+void destroy(_ForwardIterator __first, _ForwardIterator __last) {
+    for (; __first != __last; ++__first)
+        _VSTD::destroy_at(_VSTD::addressof(*__first));
+}
+
+template <class _ForwardIterator, class _Size>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+_ForwardIterator destroy_n(_ForwardIterator __first, _Size __n) {
+    for (; __n > 0; (void)++__first, --__n)
+        _VSTD::destroy_at(_VSTD::addressof(*__first));
+    return __first;
+}
+
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___MEMORY_CONSTRUCT_AT_H

libcxx/include/__memory/uninitialized_algorithms.h

#ifndef _LIBCPP_NO_EXCEPTIONS
     }
 #endif
     return __f;
 }
 
 #if _LIBCPP_STD_VER > 14
 
 template <class _ForwardIterator>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-void destroy(_ForwardIterator __first, _ForwardIterator __last) {
-    for (; __first != __last; ++__first)
-        _VSTD::destroy_at(_VSTD::addressof(*__first));
-}
-
-template <class _ForwardIterator, class _Size>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-_ForwardIterator destroy_n(_ForwardIterator __first, _Size __n) {
-    for (; __n > 0; (void)++__first, --__n)
-        _VSTD::destroy_at(_VSTD::addressof(*__first));
-    return __first;
-}
-
-template <class _ForwardIterator>
 inline _LIBCPP_INLINE_VISIBILITY
 void uninitialized_default_construct(_ForwardIterator __first, _ForwardIterator __last) {
     using _Vt = typename iterator_traits<_ForwardIterator>::value_type;
     auto __idx = __first;
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try {
 #endif
     for (; __idx != __last; ++__idx)

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy.pass.cpp

 
 // <memory>
 
 // template <class ForwardIt>
 // constexpr void destroy(ForwardIt, ForwardIt);
 
 #include <memory>
 #include <cassert>
+#include <type_traits>
 
 #include "test_macros.h"
 #include "test_iterators.h"
 
 struct Counted {
     int* counter_;
     TEST_CONSTEXPR Counted(int* counter) : counter_(counter) { ++*counter_; }
     TEST_CONSTEXPR Counted(Counted const& other) : counter_(other.counter_) { ++*counter_; }
     TEST_CONSTEXPR_CXX20 ~Counted() { --*counter_; }
     friend void operator&(Counted) = delete;
 };
 
-TEST_CONSTEXPR_CXX20 bool test()
+#if TEST_STD_VER > 17
+constexpr bool test_arrays() {
     {
+        using Array = Counted[3];
+        using Alloc = std::allocator<Array>;
+        int counter = 0;
+        Alloc alloc;
+        Array* pool = std::allocator_traits<Alloc>::allocate(alloc, 5);
+
+        for (Array* p = pool; p != pool + 5; ++p) {
+            Array& arr = *p;
+            for (int i = 0; i != 3; ++i) {
+                std::allocator_traits<Alloc>::construct(alloc, std::addressof(arr[i]), &counter);
+            }
+        }
+        assert(counter == 5 * 3);
+
+        std::destroy(pool, pool + 5);
+        ASSERT_SAME_TYPE(decltype(std::destroy(pool, pool + 5)), void);
+        assert(counter == 0);
+
+        std::allocator_traits<Alloc>::deallocate(alloc, pool, 5);
+    }
+    {
+        using Array = Counted[3][2];
+        using Alloc = std::allocator<Array>;
+        int counter = 0;
+        Alloc alloc;
+        Array* pool = std::allocator_traits<Alloc>::allocate(alloc, 5);
+
+        for (Array* p = pool; p != pool + 5; ++p) {
+            Array& arr = *p;
+            for (int i = 0; i != 3; ++i) {
+                for (int j = 0; j != 2; ++j) {
+                    std::allocator_traits<Alloc>::construct(alloc, std::addressof(arr[i][j]), &counter);
+                }
+            }
+        }
+        assert(counter == 5 * 3 * 2);
+
+        std::destroy(pool, pool + 5);
+        ASSERT_SAME_TYPE(decltype(std::destroy(pool, pool + 5)), void);
+        assert(counter == 0);
+
+        std::allocator_traits<Alloc>::deallocate(alloc, pool, 5);
+    }
+
+    return true;
+}
+#endif
+
+template <class It>
+TEST_CONSTEXPR_CXX20 void test() {
     using Alloc = std::allocator<Counted>;
     int counter = 0;
-    int const N = 5;
     Alloc alloc;
-    Counted* pool = std::allocator_traits<Alloc>::allocate(alloc, N);
+    Counted* pool = std::allocator_traits<Alloc>::allocate(alloc, 5);
 
-    for (Counted* p = pool; p != pool + N; ++p)
+    for (Counted* p = pool; p != pool + 5; ++p)
         std::allocator_traits<Alloc>::construct(alloc, p, &counter);
     assert(counter == 5);
 
-    std::destroy(pool, pool + 1);
-    assert(counter == 4);
-
-    std::destroy(forward_iterator<Counted*>(pool + 1), forward_iterator<Counted*>(pool + 5));
+    std::destroy(It(pool), It(pool + 5));
+    ASSERT_SAME_TYPE(decltype(std::destroy(It(pool), It(pool + 5))), void);
     assert(counter == 0);
 
-    std::allocator_traits<Alloc>::deallocate(alloc, pool, N);
+    std::allocator_traits<Alloc>::deallocate(alloc, pool, 5);
+}
 
+TEST_CONSTEXPR_CXX20 bool tests() {
+    test<Counted*>();
+    test<forward_iterator<Counted*>>();
     return true;
 }
 
-int main(int, char**)
-{
-    test();
+int main(int, char**) {
+    tests();
 #if TEST_STD_VER > 17
-    static_assert(test());
+    test_arrays();
+    static_assert(tests());
+    // TODO: Until std::construct_at has support for arrays, it's impossible to test this
+    //       in a constexpr context.
+    // static_assert(test_arrays());
 #endif
     return 0;
 }

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy_at.pass.cpp

 
 // <memory>
 
 // template <class T>
 // constexpr void destroy_at(T*);
 
 #include <memory>
 #include <cassert>
+#include <type_traits>
 
 #include "test_macros.h"
 
 struct Counted {
     int* counter_;
     TEST_CONSTEXPR Counted(int* counter) : counter_(counter) { ++*counter_; }
     TEST_CONSTEXPR_CXX20 ~Counted() { --*counter_; }
     friend void operator&(Counted) = delete;
 };
 
 struct VirtualCounted {
     int* counter_;
     TEST_CONSTEXPR VirtualCounted(int* counter) : counter_(counter) { ++*counter_; }
     TEST_CONSTEXPR_CXX20 virtual ~VirtualCounted() { --*counter_; }
     friend void operator&(VirtualCounted) = delete;
 };
 
 struct DerivedCounted : VirtualCounted {
     TEST_CONSTEXPR DerivedCounted(int* counter) : VirtualCounted(counter) { }
     TEST_CONSTEXPR_CXX20 ~DerivedCounted() override { }
     friend void operator&(DerivedCounted) = delete;
 };
 
-TEST_CONSTEXPR_CXX20 bool test()
+#if TEST_STD_VER > 17
+constexpr bool test_arrays() {
     {
+        using Array = Counted[3];
+        using Alloc = std::allocator<Array>;
+        Alloc alloc;
+        Array* ptr = std::allocator_traits<Alloc>::allocate(alloc, 1);
+        Array& arr = *ptr;
+
+        int counter = 0;
+        for (int i = 0; i != 3; ++i)
+            std::allocator_traits<Alloc>::construct(alloc, std::addressof(arr[i]), &counter);
+        assert(counter == 3);
+
+        std::destroy_at(ptr);
+        ASSERT_SAME_TYPE(decltype(std::destroy_at(ptr)), void);
+        assert(counter == 0);
+
+        std::allocator_traits<Alloc>::deallocate(alloc, ptr, 1);
+    }
+    {
+        using Array = Counted[3][2];
+        using Alloc = std::allocator<Array>;
+        Alloc alloc;
+        Array* ptr = std::allocator_traits<Alloc>::allocate(alloc, 1);
+        Array& arr = *ptr;
+
+        int counter = 0;
+        for (int i = 0; i != 3; ++i)
+            for (int j = 0; j != 2; ++j)
+                std::allocator_traits<Alloc>::construct(alloc, std::addressof(arr[i][j]), &counter);
+        assert(counter == 3 * 2);
+
+        std::destroy_at(ptr);
+        ASSERT_SAME_TYPE(decltype(std::destroy_at(ptr)), void);
+        assert(counter == 0);
+
+        std::allocator_traits<Alloc>::deallocate(alloc, ptr, 1);
+    }
+    return true;
+}
+#endif
+
+TEST_CONSTEXPR_CXX20 bool test() {
     {
         using Alloc = std::allocator<Counted>;
         Alloc alloc;
         Counted* ptr1 = std::allocator_traits<Alloc>::allocate(alloc, 1);
         Counted* ptr2 = std::allocator_traits<Alloc>::allocate(alloc, 1);
 
         int counter = 0;
         std::allocator_traits<Alloc>::construct(alloc, ptr1, &counter);
         std::allocator_traits<Alloc>::construct(alloc, ptr2, &counter);
         assert(counter == 2);
 
         std::destroy_at(ptr1);
+        ASSERT_SAME_TYPE(decltype(std::destroy_at(ptr1)), void);
         assert(counter == 1);
 
         std::destroy_at(ptr2);
         assert(counter == 0);
 
         std::allocator_traits<Alloc>::deallocate(alloc, ptr1, 1);
         std::allocator_traits<Alloc>::deallocate(alloc, ptr2, 1);
     }
     {
         using Alloc = std::allocator<DerivedCounted>;
         Alloc alloc;
         DerivedCounted* ptr1 = std::allocator_traits<Alloc>::allocate(alloc, 1);
         DerivedCounted* ptr2 = std::allocator_traits<Alloc>::allocate(alloc, 1);
 
         int counter = 0;
         std::allocator_traits<Alloc>::construct(alloc, ptr1, &counter);
         std::allocator_traits<Alloc>::construct(alloc, ptr2, &counter);
         assert(counter == 2);
 
         std::destroy_at(ptr1);
+        ASSERT_SAME_TYPE(decltype(std::destroy_at(ptr1)), void);
         assert(counter == 1);
 
         std::destroy_at(ptr2);
         assert(counter == 0);
 
         std::allocator_traits<Alloc>::deallocate(alloc, ptr1, 1);
         std::allocator_traits<Alloc>::deallocate(alloc, ptr2, 1);
     }
 
     return true;
 }
 
-int main(int, char**)
-{
+int main(int, char**) {
     test();
 #if TEST_STD_VER > 17
+    test_arrays();
     static_assert(test());
+    // TODO: Until std::construct_at has support for arrays, it's impossible to test this
+    //       in a constexpr context.
+    // static_assert(test_arrays());
 #endif
     return 0;
 }

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy_n.pass.cpp

 
 // <memory>
 
 // template <class ForwardIt, class Size>
 // constexpr ForwardIt destroy_n(ForwardIt, Size s);
 
 #include <memory>
 #include <cassert>
+#include <type_traits>
 
 #include "test_macros.h"
 #include "test_iterators.h"
 
 struct Counted {
     int* counter_;
     TEST_CONSTEXPR Counted(int* counter) : counter_(counter) { ++*counter_; }
     TEST_CONSTEXPR Counted(Counted const& other) : counter_(other.counter_) { ++*counter_; }
     TEST_CONSTEXPR_CXX20 ~Counted() { --*counter_; }
     friend void operator&(Counted) = delete;
 };
 
-TEST_CONSTEXPR_CXX20 bool test()
+#if TEST_STD_VER > 17
+constexpr bool test_arrays()  {
     {
+        using Array = Counted[3];
+        using Alloc = std::allocator<Array>;
+        int counter = 0;
+        Alloc alloc;
+        Array* pool = std::allocator_traits<Alloc>::allocate(alloc, 5);
+
+        for (Array* p = pool; p != pool + 5; ++p) {
+            Array& arr = *p;
+            for (int i = 0; i != 3; ++i) {
+                std::allocator_traits<Alloc>::construct(alloc, std::addressof(arr[i]), &counter);
+            }
+        }
+        assert(counter == 5 * 3);
+
+        Array* p = std::destroy_n(pool, 5);
+        ASSERT_SAME_TYPE(decltype(std::destroy_n(pool, 5)), Array*);
+        assert(p == pool + 5);
+        assert(counter == 0);
+
+        std::allocator_traits<Alloc>::deallocate(alloc, pool, 5);
+    }
+    {
+        using Array = Counted[3][2];
+        using Alloc = std::allocator<Array>;
+        int counter = 0;
+        Alloc alloc;
+        Array* pool = std::allocator_traits<Alloc>::allocate(alloc, 5);
+
+        for (Array* p = pool; p != pool + 5; ++p) {
+            Array& arr = *p;
+            for (int i = 0; i != 3; ++i) {
+                for (int j = 0; j != 2; ++j) {
+                    std::allocator_traits<Alloc>::construct(alloc, std::addressof(arr[i][j]), &counter);
+                }
+            }
+        }
+        assert(counter == 5 * 3 * 2);
+
+        Array* p = std::destroy_n(pool, 5);
+        ASSERT_SAME_TYPE(decltype(std::destroy_n(pool, 5)), Array*);
+        assert(p == pool + 5);
+        assert(counter == 0);
+
+        std::allocator_traits<Alloc>::deallocate(alloc, pool, 5);
+    }
+
+    return true;
+}
+#endif
+
+template <class It>
+TEST_CONSTEXPR_CXX20 void test() {
     using Alloc = std::allocator<Counted>;
     int counter = 0;
-    int const N = 5;
     Alloc alloc;
-    Counted* pool = std::allocator_traits<Alloc>::allocate(alloc, N);
+    Counted* pool = std::allocator_traits<Alloc>::allocate(alloc, 5);
 
-    for (Counted* p = pool; p != pool + N; ++p)
+    for (Counted* p = pool; p != pool + 5; ++p)
         std::allocator_traits<Alloc>::construct(alloc, p, &counter);
     assert(counter == 5);
 
-    Counted* np = std::destroy_n(pool, 1);
-    assert(np == pool + 1);
-    assert(counter == 4);
-
-    forward_iterator<Counted*> it = std::destroy_n(forward_iterator<Counted*>(pool + 1), 4);
-    assert(it == forward_iterator<Counted*>(pool + 5));
+    It it = std::destroy_n(It(pool), 5);
+    ASSERT_SAME_TYPE(decltype(std::destroy_n(It(pool), 5)), It);
+    assert(it == It(pool + 5));
     assert(counter == 0);
 
-    std::allocator_traits<Alloc>::deallocate(alloc, pool, N);
+    std::allocator_traits<Alloc>::deallocate(alloc, pool, 5);
+}
 
+TEST_CONSTEXPR_CXX20 bool tests() {
+    test<Counted*>();
+    test<forward_iterator<Counted*>>();
     return true;
 }
 
-int main(int, char**)
-{
-    test();
+int main(int, char**) {
+    tests();
 #if TEST_STD_VER > 17
-    static_assert(test());
+    test_arrays();
+    static_assert(tests());
+    // TODO: Until std::construct_at has support for arrays, it's impossible to test this
+    //       in a constexpr context.
+    // static_assert(test_arrays());
 #endif
     return 0;
 }