[libcxx-commits] [PATCH] D130404: [libc++][ranges] implement `std::ranges::unique{_copy}`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Thanks a lot for the patch @huixie90! Since you're on vacation, @var-const can you please commandeer to finish this up? LGTM w/ comments applied.



================
Comment at: libcxx/include/__algorithm/adjacent_find.h:28-38
   if (__first != __last) {
-    _ForwardIterator __i = __first;
+    _Iter __i = __first;
     while (++__i != __last) {
       if (__pred(*__first, *__i))
         return __first;
       __first = __i;
     }
----------------
How about this? It's equivalent, but I find it slightly easier to read.

Non-blocking if you disagree.


================
Comment at: libcxx/include/__algorithm/ranges_unique_copy.h:95
+         indirectly_copyable_storable<iterator_t<_Range>, _OutIter>)
+            _LIBCPP_HIDE_FROM_ABI constexpr unique_copy_result<borrowed_iterator_t<_Range>, _OutIter>
+            operator()(_Range&& __range, _OutIter __result, _Comp __comp = {}, _Proj __proj = {}) const {
----------------
FWIW this formatting is really hard to read for me, I assume it's produced by `clang-format`?


================
Comment at: libcxx/include/__algorithm/unique_copy.h:37
+template <class _AlgPolicy, class _BinaryPredicate, class _InputIterator, class _Sent, class _OutputIterator>
+_LIBCPP_CONSTEXPR_AFTER_CXX17 _LIBCPP_HIDE_FROM_ABI pair<_InputIterator, _OutputIterator> __unique_copy_impl(
+    _InputIterator __first,
----------------
I think we should call it `__unique_copy` instead for consistency with other algorithms (even though I understand we're passing an additional tag parameter).


================
Comment at: libcxx/include/__algorithm/unique_copy.h:47
-_LIBCPP_CONSTEXPR_AFTER_CXX17 _OutputIterator
-__unique_copy(_ForwardIterator __first, _ForwardIterator __last, _OutputIterator __result, _BinaryPredicate __pred,
-              forward_iterator_tag, output_iterator_tag)
----------------
Do we have a test for calling `unique_copy` with a `value_type` that is not copyable? IIUC that would be valid when the input range is at least a `forward_iterator`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130404