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

Mark de Wever via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Jun 1 11:45:36 PDT 2022

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?

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list