[libcxx-commits] [PATCH] D105456: [libcxx][algorithms] adds `std::ranges::find`

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added inline comments.


================
Comment at: libcxx/include/__algorithm/find.h:37
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+_Ip __find(_Ip __first, const _Sp __last, const _Tp& __value, _Proj __proj = {})
+{
----------------
I find it a bad idea to pessimize the classic algorithm

Usually the algorithms are simple enough that code duplication is not an issue


================
Comment at: libcxx/include/__algorithm/find.h:57
+  struct __find_fn final : __function_like {
+    constexpr explicit __find_fn(__tag __x) noexcept : __function_like(__x) {}
+
----------------
Would it make sense to simply do `using __function_like :.__function_like;`


================
Comment at: libcxx/include/__algorithm/find.h:63
+    {
+      return _VSTD::__find(std::move(__first), std::move(__last), __value, std::ref(__proj));
+    }
----------------
plain `std::` qualifier should be `_VSTD::`


================
Comment at: libcxx/include/__algorithm/find.h:70
+    {
+      return _VSTD::__find(ranges::begin(__r), ranges::end(__r), __value, std::ref(proj));
+    }
----------------
plain `ranges::` qualifier should be something else


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:55
+  {
+    auto const iterator_result = std::ranges::find(subrange.begin(), subrange.end(), projectable<int>{6});
+    assert(iterator_result.base() == range.begin() + 6);
----------------
I have trouble discerning what the individual tests actually test.

I would suggest to pull in the `auto subrange = std::ranges::subrange(I<array_t::iterator>(range.begin()), sentinel_wrapper(range.end()));` into each clause and give it a descriptive name like

`auto `find_without_projection = std::ranges::subrange(I<array_t::iterator>(range.begin()), sentinel_wrapper(range.end()));`

That way one can directly read what the test is testing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456