[libcxx-commits] [PATCH] D121248: [libc++][ranges] Implement ranges::find{, _if, _if_not}

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.
This revision is now accepted and ready to land.

LGTM (except the `sentinel_for` check -- sorry, I know it's tedious to add all those checks).



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if_not.pass.cpp:14
+
+// template<input_iterator I, sentinel_for<I> S, class Proj = identity,
+//          indirect_unary_predicate<projected<I, Proj>> Pred>
----------------
I think the `sentinel_for` constraint is still untested (if I'm not seeing it, please let me know). Sorry, I know it's annoying. Just a single pair of types where `I` satisfies `input_iterator` but not `sentinel_for<I, S>` is sufficient.


================
Comment at: libcxx/test/support/almost_satisfies_types.h:40
+static_assert(std::indirectly_readable<InputIteratorNotDerivedFrom>);
+static_assert(!std::input_iterator<InputIteratorNotDerivedFrom>);
+static_assert(!std::ranges::input_range<InputRangeNotDerivedFrom>);
----------------
Note: I think this type is sufficient for checking the `input_iterator` requirement -- i.e., a type that is an `input_or_output_iterator` but not `input_iterator` is perfect, but whether it satisfies `indirectly_readable` starts going into the "testing `input_iterator`" territory. So I'm not against also having `InputIteratorNotIndirectlyReadable` and `InputIteratorNotInputOrOutputIterator`, but IMO just `InputIteratorNotDerivedFrom` is sufficient.


================
Comment at: libcxx/test/support/almost_satisfies_types.h:96
+
+#endif
----------------
Nit: can you add a closing comment? (`// ALMOST_SATISFIES_TYPES_H`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248