[libcxx-commits] [PATCH] D126529: [libc++] Implement ranges::find_first_of

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

LGTM, modulo one comment.
Since I'm not too involved in ranges I prefer to leave the final approval to @var-const or @ldionne.



================
Comment at: libcxx/include/__algorithm/ranges_find_first_of.h:42
+                              _Proj2& __proj2) {
+    for (; __first1 != __last1; ++__first1) {
+      for (auto __j = __first2; __j != __last2; ++__j) {
----------------
philnik wrote:
> Mordante wrote:
> > I saw @ldionne recently raised some concerns regarding duplicating algorithms for the ranges and non-ranges version. Have we found a solution for that concern?
> I think we have to decide that on a case-by-case basis. There are cases where I think it's more hassle than it's worth like here and there are cases where it's a definite win, like `copy` or `move`. There are of course also cases where it's not really clear, like `binary_search`. Note that there are optimizations possible for the ranges algorithms which are simply not possible for the classical algorithms (like in `ranges::is_permutation`) and ones where making optimizations for the ranges version properly is a lot harder (like `ranges::move`).
Thanks for the info!


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find.first.of/ranges.find_first_of.pass.cpp:147
+    {
+      auto ret = std::ranges::find_first_of(a, a + 4, b, b + 1, std::ranges::greater{});
+      assert(ret == a + 2);
----------------
Mordante wrote:
> For clarity I would prefer to use `std::begin()` and `std::end()` here.
Can you do the same for similar places in the tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126529