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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/module.modulemap:241
+        header "__algorithm/find.h"
+        export __function_like
+      }
----------------
cjdb wrote:
> 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.
+1 please don't `export` things here; and please do format this module as a one-liner like all the others.


================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_extensions.pass.cpp:73-75
+#if TEST_STD_VER >= 20
+  std::ranges::find(std::begin(arr), std::end(arr), 1);
+#endif
----------------
This interleaving will become unwieldy very quickly as you add C++20-specific algorithms.
I recommend creating a new function, `test_ranges_algorithms()`. It will be small to start with, but it will eventually look very similar to this one. You might even go ahead and make that function a complete copy of this one, and just comment out the lines that don't work yet. Comment them in as they are implemented.
Compare line 182 below (`bit_cast`). I'm basically asking for that approach.


================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_extensions.verify.cpp:119-122
+#if TEST_STD_VER >= 20
+  // expected-warning-re at +1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::ranges::find(std::begin(arr), std::end(arr), 1);
+#endif
----------------
Ditto here. I recommend a new function `test_ranges_algorithms`.
However, I also would like to make my own NFC cleanup so that nodiscard_extensions.{pass,verify}.cpp can be meaningfully `diff`ed against each other; so until that happens, I don't care so much about what happens in this file.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:65-66
+
+    ASSERT_SAME_TYPE(decltype(ranges::find(ranges::subrange(range), projectable<int>{6})),
+                     ranges::dangling);
+  }
----------------
This seems incorrect. `subrange` is a borrowed range, so there shouldn't be any `dangling` here.
(Testing both lvalues and rvalues is important, though, exactly //because// the Ranges library messes with return types like this based on value category.)


================
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:
> 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.


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


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/special_function.compile.pass.cpp:40
+
+static_assert(!unqualified_lookup_works<std::pair<int, int>>);
+
----------------
De Morgan's Laws are unhappy here. You wrote `!(x works AND y works AND z works)`, but what you meant to test was `!(x works) AND !(y works) AND !(z works)`. You'll need to turn the concept on line 33 into a disjunction of four constraints. Alternatively:
```
void test_unqualified_lookup() {
    using T = std::pair<int, int>;
    T value;
    T a[5];
    auto r = std::ranges::subrange(a, a+5);
    static_assert(!requires { find(a, a+5, value); });
    static_assert(!requires { find(a, a+5, value, &T::first); });
    static_assert(!requires { find(r, value); });
    static_assert(!requires { find(r, value, &T::first); });
}
```
Notice that your iterator-pair `find` arguments don't include anything out of `std::ranges`, so it's totally expected that they fail to find `std::ranges::find` via ADL. (The fact that the iterator arguments were //returned// from a call to `std::ranges::begin(a)` doesn't matter at all. ADL cares only about the //types// of the arguments, and in this case they're plain old pointers to `std::pair`s.)
Also, won't ADL happily find `std::find`? I think the main thing you were testing in the old code was that `std::find` had no two-argument overload, so `find(r, value)` was ill-formed.


================
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();
+  }
----------------
cjdb wrote:
> 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.
Even better: use `base(x) == base(y)` in the tests, because test_iterators.h provides `base()` as a free function, so that we can also test against raw pointer types like `int*`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456