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

[Hui via Phabricator via libcxx-commits]

huixie90 added a subscriber: Quuxplusone.
huixie90 added a comment.

Also thanks to @Quuxplusone who contacted me and spotted that I missed an `if constexpr` in `ranges::unique_copy` and an unnecessary `next` call in `adjacent_find`



================
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;
     }
----------------
ldionne wrote:
> How about this? It's equivalent, but I find it slightly easier to read.
> 
> Non-blocking if you disagree.
Yes your suggestion is easier to read


================
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 {
----------------
ldionne wrote:
> FWIW this formatting is really hard to read for me, I assume it's produced by `clang-format`?
yes it is done by `clang-format`. yes it is really hard to read. I will try manually formatting it.


================
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,
----------------
ldionne wrote:
> 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).
sure will rename. the direct reason I didn't use `__unique_copy` at the first place is that I used this name for the namespace few lines above. But I can rename that namespace


================
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)
----------------
ldionne wrote:
> 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`.
good spot. we do have tests in ranges version but no tests in classic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130404