[libcxx-commits] [PATCH] D119763: [libc++][ranges] Add ranges::in_found_result

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM % comments!



================
Comment at: libcxx/include/__algorithm/in_found_result.h:21
+
+#ifndef _LIBCPP_HAS_NO_CONCEPTS
+
----------------
Same comment on the `#if` guard as in D119751.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp:55-56
+
+static_assert(std::is_same_v<decltype(std::ranges::in_found_result<int>::found), bool>);
+static_assert(std::is_same_v<decltype(std::ranges::in_found_result<int>::in), int>);
+
----------------
Consider swapping these lines into the usual `in, found` order.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp:71-73
+    [[maybe_unused]] auto res2 = static_cast<std::ranges::in_found_result<MoveOnly>>(std::move(res));
+    assert(res.in.get() == 0);
+    assert(!res.found);
----------------
My usual comment here about `[[maybe_unused]]` being a code smell. Test that `res2` got the right value.
Also, why `static_cast`? Shouldn't this just be
```
auto res2 = std::move(res);
```
?


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp:75-77
+  auto [in, found] = std::ranges::in_found_result<int>{1, true};
+  assert(in == 1);
+  assert(found);
----------------
Please use two different values here: `int(true)==1` and `bool(1)==true` so this isn't testing anything. I suggest `{2, false}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119763