[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/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
----------------
zoecarver wrote:
> Should we have a test for this requires clause? (Or maybe we do and I missed it.)
Nice catch! See `requirements.compile.pass.cpp`.


================
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
+    {
----------------
zoecarver wrote:
> `_LIBCPP_HIDE_FROM_ABI` and anything else from https://libcxx.llvm.org/Contributing.html#pre-commit-check-list
Is there a way for us to test this macro's presence/absence?


================
Comment at: libcxx/include/__algorithm/find.h:73
+
+  inline constexpr auto find = __find_fn(__function_like::__tag());
+} // namespace ranges
----------------
zoecarver wrote:
> 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. 
Unlike with CPOs, `ranges` algorithms don't have a problem here, because `find(std::ranges::dummy())` is [[ https://eel.is/c++draft/algorithms.requirements#2 | expressly forbidden ]].


================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_extensions.verify.pass.cpp:1
 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
----------------
zoecarver wrote:
> Do we need to update the docs about this?
Technically, no, since the docs don't specify `std::find`, only `find`. I wonder if that's disingenuous.


================
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>>;
+  {
----------------
zoecarver wrote:
> 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. 
Why? What does this add that's not already present?


================
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);
----------------
zoecarver wrote:
> (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. 
I don't really understand your rationale here.


================
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 {
----------------
zoecarver wrote:
> 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
> 
> :)
Done, thanks!


================
Comment at: libcxx/test/support/complexity_invocable.h:49
+  F f_;
+  std::ptrdiff_t count_ = 0;
+};
----------------
zoecarver wrote:
> Nit this could be an unsigned type. AFAIU it's never less than zero. 
Unsigned integers shouldn't be used to represent arithmetic, and they don't play nicely with signed integers (which are bread and butter for iterators).


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456