[libcxx-commits] [PATCH] D136522: [libcxx] patch for implementing ranges::find_last

[Hui via Phabricator via libcxx-commits]

huixie90 requested changes to this revision.
huixie90 added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please add tests for all the algorithms' behaviour and constraints.
You can take a look at this to get an idea about how we test algorithms
https://reviews.llvm.org/D130404


================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:29
+
+#if _LIBCPP_STD_VER > 17
+
----------------
is the paper C++23? If so, should it be
```
#if _LIBCPP_STD_VER >= 23
```
same applies to other files in the patch


================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:39
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  subrange<_Ip> operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
+    auto __pred = [&](auto&& __e) { return std::forward<decltype(__e)>(__e) == __value; };
----------------
question. can we use `static operator()` now?


================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:36
+subrange<_Ip> __find_last_if_impl(_Ip __first, _Sp __last, _Pred& __pred, _Proj& __proj) {
+  for (; __last != __first; --__last) {
+    if (std::invoke(__pred, std::invoke(__proj, *__last)))
----------------
There are several issues.

1. In general `__last` is a sentinel (as supposed to be an iterator). The only thing you can do about it is to compare with an iterator. so you cannot use `operator--` for a sentinel.
2. The algorithm's requirement is `forward_iterator` (instead of `bidirectional_iterator`). `operator--` does not exist for `forward_iteartor`. You will probably need to dispatch to different implementations depending on whether it is `common_range`, `bidirectional_range`, `sized_sentinel_for`/`sized_range`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136522