[libcxx-commits] [PATCH] D150831: [libc++] Implement ranges::ends_with

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:43
+      _Sent2 __last2,
+      _Pred __pred   = {},
+      _Proj1 __proj1 = {},
----------------
ZijunZhao wrote:
> var-const wrote:
> > This helper function doesn't need default arguments (since it's never invoked outside this file) and can take the predicate and the projections by reference (so there's no need to wrap the arguments passed to it in `std::ref` -- `std::ref` is only needed when passing those arguments to `ranges::equal`).
> Wrapped by `std::ref` only when passing args to `ranges::equal` will cause `ranges_robust_against_copying_comparators.pass.cpp` fail.
> So I only wrap them when passing args to `__ends_with_fn_impl` works.
I mean that the helper function should take the arguments by reference, then wrap them in `std::ref` before passing to other algorithms. Basically, I'm suggesting to move the wrapping from the public function into the helper function.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:39
+  static _LIBCPP_HIDE_FROM_ABI constexpr bool __ends_with_fn_impl(
+      _Iter1 __first1, _Sent1 __last1, _Iter2 __first2, _Sent2 __last2, _Pred __pred, _Proj1 __proj1, _Proj2 __proj2) {
+    auto __n1 = ranges::distance(__first1, __last1);
----------------
The helper function should take `_Pred` and `_Proj1/_Proj2` by a reference, which would avoid the need for `std::ref` when delegating. Unlike the public functions which must have the same signature as in the standard, we can use references in the internal helper.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:47
+
+    if constexpr (std::bidirectional_iterator<_Sent1>) {
+      if constexpr (std::bidirectional_iterator<_Sent2>) {
----------------
Note: we only qualify the namespace on function calls (to avoid triggering ADL). There is no need to qualify class names, concept names, etc.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:49
+      if constexpr (std::bidirectional_iterator<_Sent2>) {
+        auto __l1 = ranges::next(__first1, __n1 - 1);
+        auto __l2 = ranges::next(__first2, __n2 - 1);
----------------
How about something like
```
if constexpr (bidirectional_iterator<_Iter1> && bidirectional_iterator<_Iter2>) {
  auto __rbegin1 = std::make_reverse_iterator(__first1);
  auto __rbegin2 = std::make_reverse_iterator(__first2);
  auto __rend1 = std::make_reverse_iterator(ranges::next(__first1, __last1));
  auto __rend2 = std::make_reverse_iterator(ranges::next(__first2, __last2));
  return ranges::starts_with(__rbegin1, __rend1, __rbegin2, __rend2, std::ref(__pred), std::ref(__proj1), std::ref(__proj2));
}
```
?


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:64
+    ranges::advance(__first1, (__n1 - __n2));
+    auto __unwrapped1 = std::__unwrap_range(std::move(__first1), std::move(__last1));
+    auto __unwrapped2 = std::__unwrap_range(std::move(__first2), std::move(__last2));
----------------
Why do we have to unwrap here? Can't we delegate that to `equal`?


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:71
+        std::move(__unwrapped2.second),
+        __pred,
+        __proj1,
----------------
`std::ref` should be here.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:113
+      _Range1&& __range1, _Range2&& __range2, _Pred __pred = {}, _Proj1 __proj1 = {}, _Proj2 __proj2 = {}) const {
+    return __ends_with_fn_impl(
+        std::move(ranges::begin(__range1)),
----------------
I think we're missing the case where the ranges are sized (meaning we can get the size in constant time) but the sentinels aren't, and the iterators aren't random-access. In other words, we miss the case when we have a way to get the sizes in constant time while we still have the range objects, but once we reduce them to iterators, getting the size becomes linear.

I took a stab at it and can suggest something like the following. There are many potential microoptimizations here (in particular if we consider all the cases where one of the ranges is sized and the other is not), but personally I think we need to strike the right balance between performance and code clarity. My current thinking is:
1. If we can get the sizes from the ranges in constant time, that takes priority over everything else -- otherwise, we end up losing this bit of information down the line (in step 3). This also allows us to immediately filter out corner cases of empty ranges, or range2 being bigger than range1, in constant time;
2. Otherwise, if we can iterate the ranges in reverse and have the end iterators, that's the next priority -- this avoids any unnecessary linear work for finding the sizes;
3. Otherwise, calculate the sizes of both ranges (at worst two linear operations), advance the iterator on the first range (at worst linear), do the comparisons (linear). If any of these operations can be done in constant time, we get that without any extra code.

Pseudocode (I omitted the predicate and projections, as well as function attributes, for simplicity):
```
template <class _Iter1, class _Iter2>
bool __impl_bidirectional(_Iter1 __first1, _Iter1 __last1, _Iter2 __first2, _Iter2 __last2) {
  auto __rbegin1 = std::make_reverse_iterator(__first1);
  auto __rend1 = std::make_reverse_iterator(__last1);
  auto __rbegin2 = std::make_reverse_iterator(__first2);
  auto __rend2 = std::make_reverse_iterator(__last2);
  return ranges::starts_with(__rbegin1, __rend1, __rbegin2, __rend2);
}

template <class _Iter1, class _Sent1, class _Iter2, class _Sent2, class _Diff>
bool __impl_with_offset(_Iter1 __first1, _Sent1 __last1, _Iter2 __first2, _Sent2 __last2, _Diff __offset) {
  ranges::advance(__first1, __offset);
  return ranges::equal(__first1, __last1, __first2, __last2);
}

template <class _Iter1, class _Sent1, class _Iter2, class _Sent2>
bool __impl_with_sentinels(_Iter1 __first1, _Sent1 __last1, _Iter2 __first2, _Sent2 __last2) {
  if constexpr (bidirectional_iterator<_Sent1> && bidirectional_iterator<_Sent2>) {
    return __impl_bidirectional(__first1, __last1, __first2, __last2);

  } else {
    auto __n1 = ranges::distance(__first1, __last1);
    auto __n2 = ranges::distance(__first2, __last2);
    if (__n2 == 0) {
      return true;
    }
    if (__n2 > __n1) { // Also covers the case when `__n1` is `0`.
      return false;
    }

    return __impl_with_offset(__first1, __last1, __first2, __last2, __n1 - __n2);
}

// Public function that takes iterators
{
  return __impl_with_sentinels(__first1, __last1, __first2, __last2);
}

// Public function that takes ranges
{
  if constexpr (sized_range<_Range1> && sized_range<_Range2>) {
    auto __n1 = ranges::distance(__range1);
    auto __n2 = ranges::distance(__range2);
    if (__n2 == 0) {
      return true;
    }
    if (__n2 > __n1) { // Also covers the case when `__n1` is `0`.
      return false;
    }

    return __impl_with_offset(ranges::begin(__range1), ranges::end(__range1),
                              ranges::begin(__range2), ranges::end(__range2), __n1 - __n2);

  } else {
    return __impl_with_sentinels(ranges::begin(__range1), ranges::end(__range1),
                  ranges::begin(__range2), ranges::end(__range2));
  }
}
```


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:255
+        test_iterators<Iter1, Iter1, Iter2, Iter2>();
+      if constexpr (std::forward_iterator<Iter2>)
+        test_iterators<Iter1, sized_sentinel<Iter1>, Iter2, Iter2>();
----------------
You might want to add a comment to this branch and the next one that this is to test the `(forward_iterator<_Iter1> || sized_sentinel_for<_Sent1, _Iter1>)` condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150831