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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

This is a separate patch, but it would be good to track what algorithms need to be implemented and who's doing that.

Overall this looks good to me. I have a few comments below, though.



================
Comment at: libcxx/include/__algorithm/find.h:52
+    template<input_iterator _Ip, sentinel_for<_Ip> _Sp, class _Tp, class _Proj = identity>
+    requires indirect_binary_predicate<ranges::equal_to, projected<_Ip, _Proj>, const _Tp*>
+    _LIBCPP_NODISCARD_EXT constexpr
----------------
Should we have a test for this requires clause? (Or maybe we do and I missed it.)


================
Comment at: libcxx/include/__algorithm/find.h:54
+    _LIBCPP_NODISCARD_EXT constexpr
+    _Ip operator()(_Ip __first, const _Sp __last, const _Tp& __value, _Proj __proj = {}) const
+    {
----------------
`_LIBCPP_HIDE_FROM_ABI` and anything else from https://libcxx.llvm.org/Contributing.html#pre-commit-check-list


================
Comment at: libcxx/include/__algorithm/find.h:73
+
+  inline constexpr auto find = __find_fn(__function_like::__tag());
+} // namespace ranges
----------------
Can you add a test for something like this:
```
namespace std { namespace ranges {

struct dummy {
  friend void find() { }
};

}}
```
Or at least fix the problem that it will uncover. 


================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_extensions.verify.pass.cpp:1
 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
----------------
Do we need to update the docs about this?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/find_iter_sent_proj.pass.cpp:40
+{
+  using iterator = I<ranges::iterator_t<R>>;
+  {
----------------
Hmm, this isn't a big thing, but it might be nice to have something like `ASSERT_SAME_TYPE(decltype(find), int*)` where the second argument is a concrete iterator type.

At least add an `ASSERT_SAME_TYPE `, please. 


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/find_iter_sent_proj.pass.cpp:44
+      auto input = make_archetype_range<I>(r);
+      std::same_as<iterator> auto const result = ranges::find(ranges::begin(input), ranges::end(input), value, proj);
+      assert(*result == expected_value);
----------------
(Sorry to be a bit pedantic here, but...)

I think it would be simpler to just have an auto here and then assert the type below. 

The reason I think this is then two different lines are testing two different things. One line has the test for what type the return type should have, the other line has the test for what value the return type should have. I think it makes it a bit easier to parse quickly, and if there was an error, it would be clearer what the error was. 


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/special_function.compile.pass.cpp:30
+namespace std::ranges {
+  template<class T>
+  class basic_istream_view {
----------------
cjdb wrote:
> ldionne wrote:
> > Is there any way you can avoid doing this?
> I need a standard range adaptor that's guaranteed to not be a `common_range` upon inspection. Till we get `iota_view` or `basic_istream_view` (the simplest two), I think not.
Hopefully you'll have it in a few days: https://reviews.llvm.org/D107096

:)


================
Comment at: libcxx/test/support/complexity_invocable.h:49
+  F f_;
+  std::ptrdiff_t count_ = 0;
+};
----------------
Nit this could be an unsigned type. AFAIU it's never less than zero. 


================
Comment at: libcxx/test/support/test_range.h:65
+template<template<class...> class I, class R>
+constexpr auto make_archetype_range(R&& r)
+{
----------------
This should probably go in `test_ranges.h`


================
Comment at: libcxx/test/support/test_range.h:67
+{
+  return std::ranges::subrange(I(std::ranges::begin(r)), sentinel_wrapper(std::ranges::end(r)));
+}
----------------
Can we make this a move-only type? Currently I don't see any tests for move only ranges.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456