[libcxx-commits] [PATCH] D126616: [libc++] Implement ranges::move{, _backward}

Fri Jun 3 21:15:27 PDT 2022

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.

================
Comment at: libcxx/include/__algorithm/move.h:37
+  }
+  return pair<_InIter, _OutIter>(std::move(__first), std::move(__result));
 }
----------------
Nit: you can use `std::make_pair`.

================
Comment at: libcxx/include/__algorithm/move.h:40

-template <class _InputIterator, class _OutputIterator>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-_OutputIterator
-__move(_InputIterator __first, _InputIterator __last, _OutputIterator __result)
-{
-    return _VSTD::__move_constexpr(__first, __last, __result);
+template <class _InValueT,
+          class _OutValueT,
----------------
Nit: I think we usually don't add `T` to template type names.

================
Comment at: libcxx/include/__algorithm/move.h:49
+#ifndef _LIBCPP_COMPILER_GCC
+   && !is_trivially_copyable<_InValueT>::value
+#endif
----------------
Can you provide a bit more context about this workaround?

================
Comment at: libcxx/include/__algorithm/move.h:54
+  const size_t __n = static_cast<size_t>(__last - __first);
+  if (__n > 0)
+    ::__builtin_memmove(__result, __first, __n * sizeof(_OutValueT));
----------------
Is this check necessary? Does `__builtin_memmove` require the argument not to be zero?

================
Comment at: libcxx/include/__algorithm/move.h:74
+                             && __is_trivially_move_assignable_unwrapped<_InIter>::value
+                             && __is_trivially_move_assignable_unwrapped<_OutIter>::value> >
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
----------------
Question: if `_InIter` and `_OutIter` have the same underlying value type, wouldn't `__is_trivially_move_assignable_unwrapped` always return the same result for both of them?

================
Comment at: libcxx/include/__algorithm/move.h:77
+pair<reverse_iterator<_InIter>, reverse_iterator<_OutIter>>
+__move_impl(reverse_iterator<_InIter> __first,
+            reverse_iterator<_InIter> __last,
----------------
Question: this is a new optimization that isn't directly related to implementing `ranges::move`, right?

================
Comment at: libcxx/include/__algorithm/move.h:85
+  std::__move_impl(__last_base, __first_base, __result_first);
+  return std::make_pair(__last, reverse_iterator<_OutIter>(std::__rewrap_iter(__result.base(), __result_first)));
+}
----------------
Very optional: you can use `make_reverse_iterator`.

================
Comment at: libcxx/include/__algorithm/move.h:91
+pair<reverse_iterator<reverse_iterator<_InIter>>, reverse_iterator<reverse_iterator<_OutIter> > >
+__copy_impl(reverse_iterator<reverse_iterator<_InIter> > __first,
+            reverse_iterator<reverse_iterator<_InIter> > __last,
----------------
Is this used? (same for this function's implementation below)

================
Comment at: libcxx/include/__algorithm/move.h:126

 template <class _InputIterator, class _OutputIterator>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
Nit: this should be `_InIter` for consistency (I'd personally prefer `_InputIter`, but feel free to ignore).

================
Comment at: libcxx/include/__algorithm/move_backward.h:28
 {
-    if (__libcpp_is_constant_evaluated()) {
-        return _VSTD::__move_backward_constexpr(__first, __last, __result);
-    } else {
-        return _VSTD::__rewrap_iter(__result,
-            _VSTD::__move_backward(_VSTD::__unwrap_iter(__first),
-                                   _VSTD::__unwrap_iter(__last),
-                                   _VSTD::__unwrap_iter(__result)));
-    }
+  return std::__move(reverse_iterator<_BidirectionalIterator1>(__last),
+                     reverse_iterator<_BidirectionalIterator1>(__first),
----------------
I'd rather keep the previous approach -- it's more verbose, but also straightforward, symmetrical with `move.h`, and avoiding the extra overhead from using reverse iterators (which includes the additional mental burden when reading the code and more complicated types when debugging).

================
Comment at: libcxx/include/__algorithm/ranges_move.h:20
+#include <__ranges/dangling.h>
+#include <__utility/declval.h>
+#include <__utility/move.h>
----------------
Nit: seems unused (in the other ranges header as well).

================
Comment at: libcxx/include/__algorithm/ranges_move.h:40
+  template <class _InIter, class _Sent, class _OutIter>
+    requires __iter_move::__move_deref<_InIter>
+  _LIBCPP_HIDE_FROM_ABI constexpr static
----------------
Question: can you provide more context on this constraint?

================
Comment at: libcxx/include/__algorithm/ranges_move.h:55
+    }
+    return {__first, __result};
+  }
----------------
Nit: move the iterators?

================
Comment at: libcxx/include/__algorithm/ranges_move_backward.h:45
+  move_backward_result<_InIter, _OutIter> __move_backward_impl(_InIter __first, _Sent __last, _OutIter __result) {
+    auto __ret = std::__move(std::make_reverse_iterator(ranges::next(__first, __last)),
+                             std::make_reverse_iterator(__first),
----------------
Why does this function forward to `__move` with reverse iterators rather than `__move_backward`? (I understand that `__move_backward` would have to be added) While this would be more boilerplate, I think it would be simpler, more consistent, and avoid the extra overhead from using reverse iterators (and probably the need to treat them specially in `__move`).

================
Comment at: libcxx/include/__algorithm/ranges_move_backward.h:72
+  move_backward_result<borrowed_iterator_t<_Range>, _Iter> operator()(_Range&& __range, _Iter __result) const {
+    return __move_backward_impl(ranges::begin(__range), ranges::end(__range), __result);
+  }
----------------
Nit: move `result`.

================
Comment at: libcxx/include/algorithm:352
+    constexpr ranges::copy_backward_result<I1, I2>
+      ranges::move_backward(I1 first, S1 last, I2 result);                                  // since C++20
+
----------------
Please double-check the synopsis, there are a few mentions of `copy` that should be `move` (e.g., `s/copy_backward_result/move_backward_result/`).

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:15
+// template<input_iterator I, sentinel_for<I> S, weakly_incrementable O>
+//   requires indirectly_copyable<I, O>
+//   constexpr ranges::copy_result<I, O> ranges::move(I first, S last, O result);
----------------
Same as synopsis, there are a few mentions of `copy` (in the other test file as well).

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:38
+struct NotIndirectlyCopyable {};
+static_assert(!HasMoveIt<int*, NotIndirectlyCopyable*>);
+static_assert(!HasMoveIt<int*, int*, SentinelForNotSemiregular>);
----------------
Question: are these types for checking the `weakly_incrementable` constraint? Just clarifying because the names seem unrelated.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:52
+static_assert(!HasMoveR<InputRangeNotSentinelSemiregular, int*>);
+static_assert(!HasMoveR<InputRangeNotSentinelEqualityComparableWith, int*>);
+
----------------
Please also check the `weakly_incrementable` constraint.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:94
+
+struct IterMoveIter {
+  using value_type = int;
----------------
How about a name like `IteratorWithIterMove` to make it a little easier to parse?

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:101
+
+  constexpr int& operator*() const; // iterator with iter_move should not be derefernced
+
----------------
Nit: `s/derefernced/dereferenced/`.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:111
+
+constexpr bool test() {
+  test_in_iterators<cpp20_output_iterator<int*>>();
----------------
Can you add a test case that deals with a move-only type?

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:121
+    std::array<int, 4> out;
+    std::same_as<std::ranges::in_out_result<std::ranges::dangling, int*>> auto ret =
+      std::ranges::move(std::array {1, 2, 3, 4}, out.data());
----------------
Nit: `decltype(auto)`.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:138
+    struct MoveOnce {
+      bool copied = false;
+      constexpr MoveOnce() = default;
----------------
Nit: `s/copied/moved/`.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:168
+      OnlyForwardsMovable* next = nullptr;
+      bool canCopy = false;
+      OnlyForwardsMovable() = default;
----------------
Nit: `s/canCopy/canMove/`.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Comments from the other test file apply here as well.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp:13
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
+
----------------
Ultranit: there's an extra blank line.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp:139
+
+  { // check that every element is copied exactly once
+    struct CopyOnce {
----------------
Nit: `s/copied/moved/`.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp:140
+  { // check that every element is copied exactly once
+    struct CopyOnce {
+      bool copied = false;
----------------
Nit: `s/CopyOnce/MoveOnce/`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126616