[libcxx-commits] [PATCH] D124079: [libc++] Implement ranges::find_end, ranges::search{, _n}
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jun 10 15:53:21 PDT 2022
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
I apologize this review took so long. Generally looks good but there are a few missing parts to address.
================
Comment at: libcxx/include/__algorithm/ranges_find_end.h:45
+ _Proj2 __proj2 = {}) const {
+ auto __ret = std::__find_end_impl(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
+ return {__ret.first, __ret.second};
----------------
Nit: move?
================
Comment at: libcxx/include/__algorithm/ranges_find_end.h:46
+ auto __ret = std::__find_end_impl(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
+ return {__ret.first, __ret.second};
+ }
----------------
Same optional nit re. moving here.
================
Comment at: libcxx/include/__algorithm/ranges_search.h:65
+ auto __first1 = ranges::begin(__range1);
+ if constexpr (sized_range<_Range2>) {
+ auto __size2 = ranges::size(__range2);
----------------
Question: would it be possible/worthwhile to do a similar optimization for the `iterator + sentinel` overload? It seems it should be possible to use `std::size` for the case where the sentinel is the same type as the iterator, or perhaps use `subrange` (presuming the overhead is negligible and easily optimizable).
================
Comment at: libcxx/include/__algorithm/ranges_search.h:68
+ if (__size2 <= 0)
+ return {__first1, __first1};
+ if constexpr (sized_range<_Range1>) {
----------------
Hmm, I'd like to double-check this. IIUC, the relevant section is
> `{i, i + (last2 - first2)}`, where `i` is the first iterator in the range `[first1, last1 - (last2 - first2))` such that for every non-negative integer `n` less than `last2 - first2` the condition
`bool(invoke(pred, invoke(proj1, *(i + n)), invoke(proj2, *(first2 + n))))` is `true`.
However, if `last2 - first2` is zero, there exists no non-negative integer `n` less than that. It seems like it would mean that no such iterator `i` can exist and thus the algorithm should go to the next clause and return `{last1, last1}`.
================
Comment at: libcxx/include/__algorithm/ranges_search.h:78
+
+ auto __ret = std::__search_impl(ranges::begin(__range1), ranges::end(__range1),
+ ranges::begin(__range2), ranges::end(__range2),
----------------
Should this be in an `else` block of the `if constexpr`?
================
Comment at: libcxx/include/__algorithm/search.h:35
+ class _Proj2>
+_LIBCPP_CONSTEXPR_AFTER_CXX11
+pair<_Iter1, _Iter1> __search_forward_impl(_Iter1 __first1, _Sent1 __last1,
----------------
Hide from ABI?
================
Comment at: libcxx/include/__algorithm/search.h:59
if (++__m2 == __last2) // If pattern exhausted, __first1 is the answer (works for 1 element pattern)
- return _VSTD::make_pair(__first1, __m1);
- if (++__m1 == __last1) // Otherwise if source exhaused, pattern not found
- return _VSTD::make_pair(__last1, __last1);
- if (!__pred(*__m1, *__m2)) // if there is a mismatch, restart with a new __first1
+ return std::make_pair(__first1, ++__m1);
+ if (++__m1 == __last1) { // Otherwise if source exhaused, pattern not found
----------------
Question: why is `__m1` incremented? It was not incremented in the original.
================
Comment at: libcxx/include/__algorithm/search.h:150-152
+ }
+
+ return std::__search_forward_impl(__first1, __last1,
----------------
Does this need to be in an `else` branch for this code to be guaranteed to be eliminated? Perhaps you might need to add another `if constexpr (sized_sentinel_for<_Sent1, _Iter1> || sized_sentinel_for<_Sent2, _Iter2>) {` condition to wrap up the optimizations?
================
Comment at: libcxx/include/__algorithm/search.h:231
+ using __v2 = typename iterator_traits<_ForwardIterator2>::value_type;
+ return std::search(__first1, __last1, __first2, __last2, __equal_to<__v1, __v2>());
}
----------------
Nit: consider moving the iterators.
================
Comment at: libcxx/include/__algorithm/search_n.h:76
+ _SizeT __count,
+ const _Type& __value_,
+ _Pred& __pred,
----------------
Can you remove the trailing underscore from the name? (I know it's a preexistent thing)
================
Comment at: libcxx/include/__algorithm/search_n.h:80
+ _DiffT __size1) {
+ typedef typename iterator_traits<_Iter>::difference_type difference_type;
if (__count <= 0)
----------------
Can you use `using`?
================
Comment at: libcxx/include/__algorithm/search_n.h:151
+
+ return std::__search_n_forward_impl(__first, __last,
+ __count,
----------------
Same comment as in `search.h` regarding whether this should be an `else` branch of the `if constexpr`.
================
Comment at: libcxx/include/__algorithm/search_n.h:170
+ __enable_if_t<__is_cpp17_random_access_iterator<_Iter>::value>* = nullptr) {
+
+ return std::__search_n_random_access_impl(__first, __last,
----------------
Nit: unnecessary blank line.
================
Comment at: libcxx/include/__iterator/advance.h:204
+#else
+ __iter = std::move(__to);
+#endif
----------------
Should this work if `_Sent` is a different type from `_Iter`? If not, perhaps static assert?
================
Comment at: libcxx/include/__iterator/next.h:90
+#else
+ (void)__iter;
+ return __sent;
----------------
Same question re. `static_assert` here.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:333
+_LIBCPP_HIDE_FROM_ABI inline _LIBCPP_CONSTEXPR_AFTER_CXX11
+reverse_iterator<_Iter> __make_end_reverse_iterator(_Iter __first, _Sent __last) {
+#if _LIBCPP_STD_VER > 17
----------------
I think I'd rather avoid adding these two functions. `__make_begin_reverse_iterator` seems to exist only for symmetry, but more importantly, the difference between language versions that is being abstracted away by `__make_end_reverse_iterator` is not specific to reverse iterators. How about something like
```
template <class _Iter, class _Sent>
decltype(auto) __end_iterator(const _Iter& __first, _Sent& __last) {
#if _LIBCPP_STD_VER > 17
return ranges::next(__first, __last);
#else
static_assert(is_same<_Iter, _Sent>::value, "Iterator and Sentinel have to be the same type pre-C++20");
return __last;
#endif
}
```
and then use this function when creating the reverse iterators at the call site?
================
Comment at: libcxx/include/algorithm:965
#include <__algorithm/ranges_find.h>
+#include <__algorithm/ranges_find_end.h>
#include <__algorithm/ranges_find_if.h>
----------------
Please update the synopsis.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.end/ranges.find_end.pass.cpp:9
+
+// <algorithm>
+
----------------
Missing synopsis.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.end/ranges.find_end.pass.cpp:19
+#include "test_iterators.h"
+
+template <class Iter1, class Sent1, class Iter2, class Sent2 = Iter2>
----------------
Please check SFINAE as well.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.end/ranges.find_end.pass.cpp:24
+ {
+ int a[] = {1, 2, 3, 4, 5, 6};
+ int p[] = {3, 4};
----------------
Consider a helper function to take care of both the iterator overload and the range overload (can be done in a follow-up if you prefer).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.end/ranges.find_end.pass.cpp:282
+ S p[] = {{2}, {3}};
+ auto ret = std::ranges::search(a, a + 4, p, p + 2, &S::compare, &S::identity, &S::identity);
+ assert(ret.begin() == a + 1);
----------------
Should be `find_end`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.end/ranges.find_end.pass.cpp:296
+ { // check that std::ranges::dangling is returned
+ [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
+ std::ranges::search(std::array {1, 2, 3, 4}, std::array {2, 3});
----------------
Nit: `decltype(auto)`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.search/ranges.search_n.pass.cpp:249
+ [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
+ std::ranges::search(std::array {1, 2, 3, 4}, std::array {2, 3});
+ }
----------------
Should be `search_n`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124079/new/
https://reviews.llvm.org/D124079
More information about the libcxx-commits
mailing list