[libcxx-commits] [PATCH] D124079: [libc++] Implement ranges::find_end, ranges::search{, _n}

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_search.h:65
+    auto __first1 = ranges::begin(__range1);
+    if constexpr (sized_range<_Range2>) {
+      auto __size2 = ranges::size(__range2);
----------------
var-const wrote:
> 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).
This was already done in `search.h`. I've moved it now here, since it would have broken the classic algorithms, as I now know.


================
Comment at: libcxx/include/__algorithm/ranges_search.h:68
+      if (__size2 <= 0)
+        return {__first1, __first1};
+      if constexpr (sized_range<_Range1>) {
----------------
var-const wrote:
> 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}`.
Yes, the non-negative integers less than zero is the empty set. Normally "all of an empty set" is trivially true.


================
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),
----------------
var-const wrote:
> Should this be in an `else` block of the `if constexpr`?
No. The stuff inside the `if constexpr` just checks if there even can be a match or if it's trivial because the size of `__range2` is zero, which means the beginning is always a match.


================
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
----------------
var-const wrote:
> Question: why is `__m1` incremented? It was not incremented in the original.
Yes, that was a bug in the original. The second element of pair was never used anywhere. I don't know why `__search` ever returned a pair.


================
Comment at: libcxx/include/__algorithm/search.h:150-152
+  }
+
+  return std::__search_forward_impl(__first1, __last1,
----------------
var-const wrote:
> 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?
An `else` branch would be required for the guarantee, but it makes the code a lot more complicated and clang eliminated dead code even at -O0, so I don't se much of an upside.


================
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>());
 }
----------------
var-const wrote:
> Nit: consider moving the iterators.
I would avoid moving anything in the classic algorithm parts. It probably doesn't do much and might be a portability pit-fall.


================
Comment at: libcxx/include/__algorithm/search_n.h:151
+
+  return std::__search_n_forward_impl(__first, __last,
+                                      __count,
----------------
var-const wrote:
> Same comment as in `search.h` regarding whether this should be an `else` branch of the `if constexpr`.
Answer is also the same.


================
Comment at: libcxx/include/__iterator/advance.h:204
+#else
+  __iter = std::move(__to);
+#endif
----------------
var-const wrote:
> Should this work if `_Sent` is a different type from `_Iter`? If not, perhaps static assert?
Doesn't apply anymore because I'm now using `_IterOps`.


================
Comment at: libcxx/include/__iterator/next.h:90
+#else
+  (void)__iter;
+  return __sent;
----------------
var-const wrote:
> Same question re. `static_assert` here.
Again, doesn't apply anymore because of `_IterOps`.


================
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
----------------
var-const wrote:
> 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?
Doesn't apply anymore, since we can't use `reverse_iterator` in classic algos.


================
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};
----------------
var-const wrote:
> 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).
I'll do it in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124079