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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Generally this looks good to me. I have a few small comments, nothing too major. Looking forward to shipping this!



================
Comment at: libcxx/include/__algorithm/find.h:71
+    {
+      return _VSTD::__find(ranges::begin(__r), ranges::end(__r), __value, std::ref(__proj));
+    }
----------------
The `std::` qualifier should be `_VSTD`.


================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_extensions.pass.cpp:74
+#if TEST_STD_VER >= 20
+  std::ranges::find(std::begin(arr), std::end(arr), 1);
+#endif
----------------
Nit: maybe use ranges here? (I.e., `std::ranges::begin`.)


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:50
+{
+  namespace ranges = std::ranges;
+
----------------
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.)


================
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());
----------------
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. 


================
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);
----------------
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). 


================
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);
----------------
Where's the case where the first argument is a range and the last argument is a projection? 


================
Comment at: libcxx/test/support/test_iterators.h:883
+  requires sentinel_for_base<I, I2>
+  constexpr bool operator==(I2 const& other) const {
+    return base_ == other.base();
----------------
Thanks for fixing this type :) 

I probably should have done this when I needed a similar use case rather than just writing my own sentinel. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456