[libcxx-commits] [PATCH] D121436: [libc++][ranges] Add ranges::out_value_result

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/out_value_result.h:23
+
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
----------------
I think this is no longer necessary? Should it check for C++20 and later instead?


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
----------------
I think this is no longer necessary as well.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:11
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
----------------
Hmm, but the header file isn't guarded on incomplete ranges?


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:26
+// no implicit conversion
+static_assert(!std::is_constructible_v<std::ranges::out_value_result<A, A>, std::ranges::out_value_result<int, int>>);
+
----------------
Nit: a few of these lines are longer than 120 columns.

Optional: maybe use a type alias to cut down the boilerplate? I find it a little hard to separate the useful information in each check.
```
template <class T, class U>
using Res = std::ranges::out_value_result<T, U>; // Any short name would do
```


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:35
+static_assert(std::is_constructible_v<std::ranges::out_value_result<B, B>, const std::ranges::out_value_result<int, int>>);
+static_assert(std::is_constructible_v<std::ranges::out_value_result<B, B>, const std::ranges::out_value_result<int, int>&>);
+
----------------
Question: would it make sense to also check rvalue references for good measure?


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:40
+};
+static_assert(!std::is_constructible_v<std::ranges::out_value_result<C, C>, std::ranges::out_value_result<int, int>&>);
+
----------------
Nit: a similar check above uses a value, not a reference, for the second template argument. Can this be consistent? (assuming either can be used in this case)


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:63
+
+constexpr bool test() {
+  {
----------------
A few test cases I'd like to add:
- check that `out_value_result` is trivial and standard layout when it is true of the arguments;
- check default construction;
- check that `out_value_result` is copyable and movable if the arguments are (I think a `static_assert` is enough);
- check that the rvalue overload of the conversion operator actually moves the arguments (by using a helper type that counts the number of copies and moves);
- check that the `[[no_unique_address]]` optimization is used;
- maybe check aggregate initialization (might be overkill).


================
Comment at: libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp:64
+constexpr bool test() {
+  {
+    std::ranges::out_value_result<int, double> res{10L, 0.};
----------------
Can you please add a comment to each test case that briefly explains what it does?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121436