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

[Louis Dionne via Phabricator via libcxx-commits]

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


================
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 = {})
+{
----------------
cjdb wrote:
> 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.
So the general idea is to avoid duplicating non trivial algorithmic logic, especially when we have optimizations in place. That's always been my goal, and that's why I originally requested that we try sharing the implementations.

Seeing this patch, I think we can agree it will have to be decided on a case-by-case basis. It's difficult to argue for sharing this simple for-loop given the added complexity.

@cjdb For something simple like this, I think it's fine to reimplement.


================
Comment at: libcxx/include/module.modulemap:241
+        header "__algorithm/find.h"
+        export __function_like
+      }
----------------
What happens if you don't do this?


================
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);
----------------
cjdb wrote:
> 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.
Sorry for the churn, but I strongly think that using "better names" makes things harder to read. Instead of trying to use descriptive names that are longer but that eventually fail to be descriptive because identifiers can't be plain english, use a simple and consistent name like `it`, but add a comment in plain english that explains what this test case is about. For example, I don't find `different_value_result` especially telling, and I don't think it's possible to explain the test case properly in the number of characters that keeps a variable name reasonable.


================
Comment at: libcxx/test/support/test_iterators.h:880-884
+  template<std::input_or_output_iterator I2>
+  requires sentinel_for_base<I, I2>
+  constexpr bool operator==(I2 const& other) const {
+    return base_ == other.base();
+  }
----------------
Why do we need to add this? This is only a convenience, right? If so, let's remove it and use `x.base() == y.base()` in the tests. We must keep our archetypes minimal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456