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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 7 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/include/module.modulemap:241
+        header "__algorithm/find.h"
+        export __function_like
+      }
----------------
ldionne wrote:
> What happens if you don't do this?
The `__function_like` properties aren't exported. It's not clear to me if this is intentional or a bug.


================
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);
----------------
ldionne wrote:
> 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.
I'm grateful.


================
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();
+  }
----------------
ldionne wrote:
> 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.
It's a requirement for `alg(cpp20_input_iterator(first), sentinel(last))`. Without it, that will fail because `cpp20_input_iterator` has the minimal `input_iterator` interface.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456