Fixes to the base facilities

Author: ldionne

libcxx/include/__memory/destroy.h

@@ -13,7 +13,6 @@
 #include <__memory/addressof.h>
 #include <__memory/allocator_traits.h>
 #include <__memory/construct_at.h>
-#include <__memory/pointer_traits.h>
 #include <__utility/move.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -48,7 +47,7 @@ template <class _Alloc, class _Iter, class _Sent>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
 __allocator_destroy(_Alloc& __alloc, _Iter __first, _Sent __last) {
   for (; __first != __last; ++__first)
-    allocator_traits<_Alloc>::destroy(__alloc, std::__to_address(__first));
+    allocator_traits<_Alloc>::destroy(__alloc, std::addressof(*__first));
 }
 
 template <class _ForwardIterator>

libcxx/include/__memory/relocate_at.h

@@ -49,7 +49,7 @@ _LIBCPP_HIDE_FROM_ABI _Tp* __libcpp_builtin_trivially_relocate_at(_Tp* __source,
 }
 
 template <class _Tp>
-_LIBCPP_HIDE_FROM_ABI _Tp* __libcpp_builtin_trivially_relocate_at(_Tp* __first, Tp* __last, _Tp* __dest) _NOEXCEPT {
+_LIBCPP_HIDE_FROM_ABI _Tp* __libcpp_builtin_trivially_relocate_at(_Tp* __first, _Tp* __last, _Tp* __dest) _NOEXCEPT {
   static_assert(__libcpp_is_trivially_relocatable<_Tp>::value, "");
   // Casting to void* to suppress clang complaining that this is technically UB.
   __builtin_memmove(static_cast<void*>(__dest), __first, (__last - __first) * sizeof(_Tp));

libcxx/include/__memory/uninitialized_relocate.h

@@ -11,7 +11,9 @@
 
 #include <__config>
 #include <__iterator/iterator_traits.h>
+#include <__memory/addressof.h>
 #include <__memory/allocator_traits.h>
+#include <__memory/destroy.h>
 #include <__memory/is_trivially_allocator_relocatable.h>
 #include <__memory/pointer_traits.h>
 #include <__memory/relocate_at.h>
@@ -30,8 +32,6 @@ _LIBCPP_PUSH_MACROS
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-// TODO: We currently use std::__to_address but we don't guarantee contiguous iterators. How does that work?
-
 // __uninitialized_relocate relocates the objects in [__first, __last) into __result.
 //
 // Relocation means that the objects in [__first, __last) are placed into __result as-if by move-construct and destroy,
@@ -49,7 +49,7 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 //  - __result contains the objects from [__first, __last)
 //  - [__first, __last) doesn't contain any objects
 template <class _InputIter, class _NothrowForwardIter>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _InputIter
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _NothrowForwardIter
 __uninitialized_relocate(_InputIter __first, _InputIter __last, _NothrowForwardIter __result) {
   if constexpr (__libcpp_is_contiguous_iterator<_InputIter>::value &&
                 __libcpp_is_contiguous_iterator<_NothrowForwardIter>::value) {
@@ -64,10 +64,12 @@ __uninitialized_relocate(_InputIter __first, _InputIter __last, _NothrowForwardI
   if constexpr (__libcpp_is_contiguous_iterator<_InputIter>::value &&
                 __libcpp_is_contiguous_iterator<_NothrowForwardIter>::value &&
                 __libcpp_is_trivially_relocatable<_ValueType>::value) {
-    // TODO: We might be able to memcpy if we don't overlap at all?
-    std::__libcpp_builtin_trivially_relocate_at(
-        std::__to_address(__first), std::__to_address(__last), std::__to_address(__result));
-    return __result + (__last - __first);
+    if (!__libcpp_is_constant_evaluated()) {
+      // TODO: We might be able to memcpy if we don't overlap at all?
+      std::__libcpp_builtin_trivially_relocate_at(
+          std::__to_address(__first), std::__to_address(__last), std::__to_address(__result));
+      return __result + (__last - __first);
+    }
   }
 
   // Otherwise, relocate elements one by one.
@@ -77,7 +79,7 @@ __uninitialized_relocate(_InputIter __first, _InputIter __last, _NothrowForwardI
   auto const __first_result = __result;
   try {
     while (__first != __last) {
-      std::__relocate_at(std::__to_address(__first), std::__to_address(__result));
+      std::__relocate_at(std::addressof(*__first), std::addressof(*__result));
       ++__first;
       ++__result;
     }
@@ -93,14 +95,13 @@ __uninitialized_relocate(_InputIter __first, _InputIter __last, _NothrowForwardI
 // the range [first, last) to another range ending at __result_last. The elements are relocated in reverse
 // order, but their relative order is preserved.
 template <class _BidirectionalIter, class _NothrowBidirectionalIter>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _BidirectionalIter __uninitialized_relocate_backward(
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _NothrowBidirectionalIter __uninitialized_relocate_backward(
     _BidirectionalIter __first, _BidirectionalIter __last, _NothrowBidirectionalIter __result_last) {
   if constexpr (__libcpp_is_contiguous_iterator<_BidirectionalIter>::value &&
                 __libcpp_is_contiguous_iterator<_NothrowBidirectionalIter>::value) {
-    // TODO: Check for off-by-one here, we might want to check __result_last - 1
     _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(
         !std::__is_pointer_in_range(
-            std::__to_address(__first), std::__to_address(__last), std::__to_address(__result_last)),
+            std::__to_address(__first), std::__to_address(__last), std::__to_address(__result_last) - 1),
         "uninitialized_relocate_backward requires the end of the result not to overlap with the input range");
   }
   using _ValueType = typename iterator_traits<_BidirectionalIter>::value_type;
@@ -110,11 +111,13 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _BidirectionalIter __uniniti
   if constexpr (__libcpp_is_contiguous_iterator<_BidirectionalIter>::value &&
                 __libcpp_is_contiguous_iterator<_NothrowBidirectionalIter>::value &&
                 __libcpp_is_trivially_relocatable<_ValueType>::value) {
-    auto __result = __result_last - (__last - __first);
-    // TODO: We might be able to memcpy if we don't overlap at all?
-    std::__libcpp_builtin_trivially_relocate_at(
-        std::__to_address(__first), std::__to_address(__last), std::__to_address(__result));
-    return __result;
+    if (!__libcpp_is_constant_evaluated()) {
+      auto __result = __result_last - (__last - __first);
+      // TODO: We might be able to memcpy if we don't overlap at all?
+      std::__libcpp_builtin_trivially_relocate_at(
+          std::__to_address(__first), std::__to_address(__last), std::__to_address(__result));
+      return __result;
+    }
   }
 
   // Otherwise, relocate elements one by one, starting from the end.
@@ -126,7 +129,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _BidirectionalIter __uniniti
     while (__last != __first) {
       --__last;
       --__result;
-      std::__relocate_at(std::__to_address(__last), std::__to_address(__result));
+      std::__relocate_at(std::addressof(*__last), std::addressof(*__result));
     }
   } catch (...) {
     std::destroy(__first, __last);
@@ -156,7 +159,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _NothrowForwardIter __uninit
     auto const __first_result = __result;
     try {
       while (__first != __last) {
-        std::__allocator_relocate_at(__alloc, std::__to_address(__first), std::__to_address(__result));
+        std::__allocator_relocate_at(__alloc, std::addressof(*__first), std::addressof(*__result));
         ++__first;
         ++__result;
       }
@@ -176,10 +179,9 @@ __uninitialized_allocator_relocate_backward(
     _Alloc& __alloc, _BidirectionalIter __first, _BidirectionalIter __last, _NothrowBidirectionalIter __result_last) {
   if constexpr (__libcpp_is_contiguous_iterator<_BidirectionalIter>::value &&
                 __libcpp_is_contiguous_iterator<_NothrowBidirectionalIter>::value) {
-    // TODO: Off by one?
     _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(
         !std::__is_pointer_in_range(
-            std::__to_address(__first), std::__to_address(__last), std::__to_address(__result_last)),
+            std::__to_address(__first), std::__to_address(__last), std::__to_address(__result_last) - 1),
         "uninitialized_allocator_relocate_backward requires the end of the result not to overlap with the input range");
   }
 
@@ -194,7 +196,7 @@ __uninitialized_allocator_relocate_backward(
       while (__last != __first) {
         --__last;
         --__result;
-        std::__allocator_relocate_at(__alloc, std::__to_address(__last), std::__to_address(__result));
+        std::__allocator_relocate_at(__alloc, std::addressof(*__last), std::addressof(*__result));
       }
     } catch (...) {
       std::__allocator_destroy(__alloc, __first, __last);

libcxx/test/libcxx/memory/uninitialized_allocator_relocate.pass.cpp

@@ -0,0 +1,183 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// Test the std::__uninitialized_allocator_relocate internal algorithm.
+
+#include <__memory/uninitialized_relocate.h>
+#include <cassert>
+#include <cstddef>
+#include <memory>
+
+#include "test_macros.h"
+#include "test_iterators.h"
+#include "min_allocator.h"
+
+template <class Allocator>
+struct Fixture {
+  using Traits    = std::allocator_traits<Allocator>;
+  using ValueType = typename Traits::value_type;
+  using Pointer   = typename Traits::pointer;
+
+  constexpr Fixture(std::size_t n) : size(n) {
+    source = allocator.allocate(n);
+    for (std::size_t i = 0; i != n; ++i) {
+      Traits::construct(allocator, std::to_address(source + i), ValueType(i));
+    }
+
+    dest = allocator.allocate(n);
+  }
+
+  constexpr void relocated(std::size_t n) { relocated_ = n; }
+
+  constexpr ~Fixture() {
+    for (std::size_t i = 0; i != relocated_; ++i) {
+      Traits::destroy(allocator, std::to_address(dest + i));
+    }
+    allocator.deallocate(dest, size);
+
+    for (std::size_t i = relocated_; i != size; ++i) {
+      Traits::destroy(allocator, std::to_address(source + i));
+    }
+    allocator.deallocate(source, size);
+  }
+
+  Allocator allocator;
+  std::size_t size;
+  std::size_t relocated_ = 0;
+  Pointer source;
+  Pointer dest;
+};
+
+template <class Alloc, class Iterator, class OutputIterator>
+constexpr void test() {
+  using T       = std::allocator_traits<Alloc>::value_type;
+  using Pointer = std::allocator_traits<Alloc>::pointer;
+
+  // Relocate an empty range
+  {
+    Fixture<Alloc> t(10);
+
+    OutputIterator res = std::__uninitialized_allocator_relocate(
+        t.allocator,
+        Iterator(std::to_address(t.source)),
+        Iterator(std::to_address(t.source)),
+        OutputIterator(std::to_address(t.dest)));
+    assert(res == OutputIterator(std::to_address(t.dest)));
+    t.relocated(0);
+
+    for (int i = 0; i != 10; ++i) {
+      assert(t.source[i] == T(i));
+    }
+  }
+
+  // Range of size 1
+  {
+    Fixture<Alloc> t(10);
+
+    OutputIterator res = std::__uninitialized_allocator_relocate(
+        t.allocator,
+        Iterator(std::to_address(t.source)),
+        Iterator(std::to_address(t.source + 1)),
+        OutputIterator(std::to_address(t.dest)));
+    assert(res == OutputIterator(std::to_address(t.dest + 1)));
+    t.relocated(1);
+
+    assert(t.dest[0] == T(0));
+    assert(t.source[1] == T(1));
+    assert(t.source[2] == T(2));
+    // ...
+  }
+
+  // Range of normal size
+  {
+    Fixture<Alloc> t(10);
+
+    OutputIterator res = std::__uninitialized_allocator_relocate(
+        t.allocator,
+        Iterator(std::to_address(t.source)),
+        Iterator(std::to_address(t.source + 10)),
+        OutputIterator(std::to_address(t.dest)));
+    assert(res == OutputIterator(std::to_address(t.dest + 10)));
+    t.relocated(10);
+
+    for (int i = 0; i != 10; ++i) {
+      assert(t.dest[i] == T(i));
+    }
+  }
+
+  // Relocate with some overlap between the input and the output range
+  {
+    Alloc allocator;
+    Pointer buff = allocator.allocate(10);
+    for (std::size_t i = 5; i != 10; ++i) {
+      std::allocator_traits<Alloc>::construct(allocator, std::to_address(buff + i), T(i));
+    }
+
+    OutputIterator res = std::__uninitialized_allocator_relocate(
+        allocator,
+        Iterator(std::to_address(buff + 5)),
+        Iterator(std::to_address(buff + 10)),
+        OutputIterator(std::to_address(buff)));
+    assert(res == OutputIterator(std::to_address(buff + 5)));
+
+    for (int i = 0; i != 5; ++i) {
+      assert(buff[i] == T(i + 5));
+      std::allocator_traits<Alloc>::destroy(allocator, std::to_address(buff + i));
+    }
+    allocator.deallocate(buff, 10);
+  }
+
+  // TODO: Add exception test
+}
+
+struct NotTriviallyRelocatable {
+  constexpr explicit NotTriviallyRelocatable(int i) : value_(i) {}
+  constexpr NotTriviallyRelocatable(NotTriviallyRelocatable&& other) : value_(other.value_) { other.value_ = -1; }
+  constexpr friend bool operator==(NotTriviallyRelocatable const& a, NotTriviallyRelocatable const& b) {
+    return a.value_ == b.value_;
+  }
+
+  int value_;
+};
+
+template <class T>
+struct ConsructAllocator : std::allocator<T> {
+  template < class... Args>
+  constexpr void construct(T* loc, Args&&... args) {
+    std::construct_at(loc, std::forward<Args>(args)...);
+  }
+};
+
+template <class T>
+struct DestroyAllocator : std::allocator<T> {
+  constexpr void destroy(T* loc) { std::destroy_at(loc); }
+};
+
+constexpr bool tests() {
+  test<std::allocator<int>, cpp17_input_iterator<int*>, forward_iterator<int*>>();
+  test<std::allocator<int>, contiguous_iterator<int*>, contiguous_iterator<int*>>();
+  test<min_allocator<int>, cpp17_input_iterator<int*>, forward_iterator<int*>>();
+  test<std::allocator<NotTriviallyRelocatable>,
+       cpp17_input_iterator<NotTriviallyRelocatable*>,
+       forward_iterator<NotTriviallyRelocatable*>>();
+  test<ConsructAllocator<NotTriviallyRelocatable>,
+       cpp17_input_iterator<NotTriviallyRelocatable*>,
+       forward_iterator<NotTriviallyRelocatable*>>();
+  test<DestroyAllocator<NotTriviallyRelocatable>,
+       cpp17_input_iterator<NotTriviallyRelocatable*>,
+       forward_iterator<NotTriviallyRelocatable*>>();
+  return true;
+}
+
+int main(int, char**) {
+  tests();
+#if TEST_STD_VER >= 20
+  static_assert(tests(), "");
+#endif
+  return 0;
+}