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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb 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 = {})
+{
----------------
miscco wrote:
> I find it a bad idea to pessimize the classic algorithm
> 
> Usually the algorithms are simple enough that code duplication is not an issue
I'm not fond of this either, but it was requested of me. ATTN @ldionne.


================
Comment at: libcxx/include/__algorithm/find.h:57
+  struct __find_fn final : __function_like {
+    constexpr explicit __find_fn(__tag __x) noexcept : __function_like(__x) {}
+
----------------
miscco wrote:
> Would it make sense to simply do `using __function_like :.__function_like;`
No, we already decided against this in `ranges::advance`.


================
Comment at: libcxx/include/__algorithm/find.h:70
+    {
+      return _VSTD::__find(ranges::begin(__r), ranges::end(__r), __value, std::ref(proj));
+    }
----------------
miscco wrote:
> plain `ranges::` qualifier should be something else
It's been this way for the entirety of ranges.


================
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);
----------------
miscco wrote:
> 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
Yes to better names, but I don't see how moving the subrange anywhere will improve readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456