[libcxx-commits] [PATCH] D130404: [libc++][ranges] implement `std::ranges::unique{_copy}`
Hui via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jul 28 13:14:10 PDT 2022
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
More information about the libcxx-commits
mailing list