[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/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);
----------------
Quuxplusone wrote:
> cjdb wrote:
> > 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.
> +100 that `iterator_result` should be spelled `it`.
> `range_result` should be spelled `r`.
> `range` should probably be spelled `input`, for clarity. Alternatively, remove the dependency on `std::array` — make it a plain old array, and name it `a`, just like in all existing algorithms tests.
> 
> It's unclear to me what the difference is between `projectable<int>` and `projectable<double>`. Let's replace `double` with `int` throughout. Having done that, let's make `Projectable` into a non-template. Having done that, let's make `check_find()` take a template type parameter `class It`, not a template template parameter: e.g. `check_find<contiguous_iterator<Projectable>>();`.
> 
> It's also not clear to me how `Projectable` differs meaningfully from `int`. `Projectable` is providing a lot of implicit conversions every which way; aren't those just re-implementing things that `int` already would have had? I think it would make more sense to define a type _`Projection`_ and then test some calls like
> ```
> template<class T>
> struct Mod10 {
>     constexpr int operator()(const T& x) const { return x % 10; }
> };
> template<class It>
> constexpr bool check_find() {
>     int a[] = {21, 22, 23, 24, 25, 23};
>     It first = It(a);
>     It last = It(a+5);
>     auto r = std::ranges::subrange(first, last);
>     {
>         It it = std::ranges::find(r, 23);
>         assert(base(it) == a+2);
>         it = std::ranges::find(r, 3);
>         assert(base(it) == a+5);
>         it = std::ranges::find(r, 3, Mod10<int>());
>         assert(base(it) == a+2);
>         it = std::ranges::find(r, 6, Mod10<int>());
>         assert(base(it) == a+5);
>     }
>     // repeat with `r` replaced by `a`, by `first, last`, possibly by `std::move(r)`, possibly etc.
>     // possibly repeat with `int` replaced by `MoveOnly`
> }
> ```
> This will be much easier to see what's going on.
> +100 that `iterator_result` should be spelled `it`.
> `range_result` should be spelled `r`.
> `range` should probably be spelled `input`, for clarity. Alternatively, remove the dependency on `std::array` — make it a plain old array, and name it `a`, just like in all existing algorithms tests.

Your opinion has been noted.

> It's unclear to me what the difference is between `projectable<int>` and `projectable<double>`. Let's replace `double` with `int` throughout. Having done that, let's make `Projectable` into a non-template. Having done that, let's make `check_find()` take a template type parameter `class It`, not a template template parameter: e.g. `check_find<contiguous_iterator<Projectable>>();`.

Considering that `find(range_of_ints, some_double)` is permitted by the interface, we need to be testing that.

The rest has been applied in some fashion.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:50
+{
+  namespace ranges = std::ranges;
+
----------------
zoecarver wrote:
> I like this. But other don't. Let's go for consistency and use `std::ranges` everywhere. 
> 
> (The reason not to use this, which is fair, is that we can't change it at any point. We can do a simple find and replace to go from std::ranges -> ranges or stdr or whatever, but we can't go from ranges:: to std::ranges.)
I'm not sure that's a strong enough argument, since the alias improves readability. A counterargument to the reason you've provided is that we can use a regex to find all aliases.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:61
+
+    auto const range_result = ranges::find(subrange, projectable<int>{6});
+    assert(range_result.base() == iterator_result.base());
----------------
zoecarver wrote:
> I think it would reduce complexity, and increase clarity in what is actually being tested if tests that had `Proj = identity` didn't use the projectable type. 
I'm not sure I understand what you're asking for here. Could you please rephrase?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:69
+  {
+    auto const iterator_result = ranges::find(subrange.begin(), subrange.end(), projectable<double>{3.0});
+    assert(iterator_result.base() == range.begin() + 3);
----------------
zoecarver wrote:
> This is testing that the conversion operator is invoked? I'm not sure you //need// to test that, but if you want to, I'd use a specific "convertible_to_int" type or something (or even just a "convertible<T>" type). 
Strictly speaking, this case could be omitted, but it's documenting that `int == double` //can// return true (which I think is important in light of the test below).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:93
+  {
+    auto const iterator_result = ranges::find(subrange.begin(), subrange.end(), 6, &projectable<int>::y);
+    assert(iterator_result.base() == range.begin() + 5);
----------------
zoecarver wrote:
> Where's the case where the first argument is a range and the last argument is a projection? 
Right below.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/special_function.compile.pass.cpp:27
+
+namespace stdr = std::ranges;
+
----------------
Quuxplusone wrote:
> Please avoid namespace aliases whenever possible. In this case I think it's pretty easy to avoid.
Your opinion has been noted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456