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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/algorithm:48-71
+  template<input_iterator I, sentinel_for<I> S, class T, class Proj = identity>
+    requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
+    constexpr I find(I first, S last, const T& value, Proj proj = {});              // since C++20
+  template<input_range R, class T, class Proj = identity>
+    requires indirect_binary_predicate<ranges::equal_to, projected<iterator_t<R>, Proj>, const T*>
+    constexpr borrowed_iterator_t<R>
+      find(R&& r, const T& value, Proj proj = {});                                  // since C++20
----------------
philnik wrote:
> ldionne wrote:
> > This looks weirdly formatted -- please try to indent the return types consistently, etc.
> I'm not sure what you are asking for. The return types are all indented the same, and this is copied 1:1 from the standard (excluding the `// since C++20`).
philnik's code is consistent with the standard's own weird formatting, which we do use in plenty of synopsis comments:
```
template<class T>
  constexpr R
    function_name(Args);
```
But immediately above this new stuff, the older stuff seems to be doing more libc++-code-style formatting:
```
template<class T>
constexpr R function_name(Args);
```
and immediately below, the //really// old stuff seems to be doing yet a third thing:
```
template<class T>
    constexpr R
    function_name(Args);
```
I personally don't mind the discrepancy, although it would be nice to make everything consistent one way or another.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:54-58
+    {
+      int a[] = {1, 2, 3, 4};
+      auto ret = std::ranges::find(a, a + 4, a + 3, [](int& i){ return &i; });
+      assert(ret == a + 3);
+    }
----------------
ldionne wrote:
> These projection tests are really clever. Something like this would be easier to read:
> 
> ```
> struct Point { int x; int y; };
> Point points[] = {....};
> auto ret = std::ranges::find(points, points + 4, 3, [](Point const& p) { return p.x; });
> assert(ret == ....);
> ```
> 
I agree it's hard to read, but part of the point of the projection test is to verify that we're calling the projection specifically on `*first`, never on a //copy// of `*first`. So "this projection returns the address of the argument" is important to the test.
However, we could do this:
```
assert(std::ranges::find(a, a + 4, 3, [](int& i) { return &i - a; }) == a + 3);
```
to avoid passing `a, a + 4, a + 3` in close proximity.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:68
+    [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
+      std::ranges::find(std::array {1, 2}, 3);
+  }
----------------
philnik wrote:
> Quuxplusone wrote:
> > 
> I get why you want to avoid CTAD in template code, but what's wrong with it here? `std::array {1, 2}` is definitely easier to read than `std::array<int, 2> {1, 2}` and there is no chance that it deduces the wrong type.
Just for simplicity... Ah, but I see, I was forgetting that `std::array<int>{1, 2}` doesn't work, it needs to be `std::array<int, 2>{1, 2}`, which is indeed another step uglier.
In that case, I'll settle for `std::array{1, 2}` with no space between the type name and the brace. :)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248