[libcxx-commits] [PATCH] D116278: [libc++][ranges] Add ranges::in_in_result

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

Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: ldionne.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/results.h:10-11
+
+#ifndef _LIBCPP___ALGORITHM_IN_IN_RESULT_H
+#define _LIBCPP___ALGORITHM_IN_IN_RESULT_H
+
----------------
This is the right header guard, but the wrong everything else. ;) I'd like to see this in a file named `__algorithm/in_in_result.h`.
It would also be useful to see a dependent PR (as a "child revision" of this one) implementing some algorithm that actually uses this type. `std::ranges::swap_ranges` would be a good candidate.


================
Comment at: libcxx/include/algorithm:20-38
+namespace ranges { // since C++20
+  template<class I1, class I2>
+  struct in_in_result {
+    [[no_unique_address]] I1 in1;
+    [[no_unique_address]] I2 in2;
+
+    template<class II1, class II2>
----------------
Defer to @ldionne's opinion on this, but I wonder if it would suffice to replace lines 20–38 with
```
template <class I1, class I2>
  struct ranges::in_in_result;  // since C++20
```
Going into the full level of detail for every single result type here in this synopsis comment is going to take hundreds of lines, for very little benefit that I can discern.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:38
+    assert(res.in2 == 0.);
+    auto res2 = static_cast<std::ranges::in_in_result<ConstructibleFrom<int>, ConstructibleFrom<double>>>(res);
+    assert(res2.in1.content == 10);
----------------
Please make this simply
```
std::ranges::in_in_result<double, int> res2 = res;
assert(res2.in1 == 10);
assert(res2.in2 == 0);
```
For simplicity/realism, and also because you need to test that the conversion is non-explicit.

You're missing tests to verify (1) that `convertible_to` is not `constructible_from`, e.g.
```
struct A { explicit A(int); };
static_assert(!std::is_constructible_v<std::ranges::in_in_result<A, A>, std::ranges::in_in_result<int, int>>);
```
and (2) that the copy constructor can still be used when the move-constructor can't, e.g.
```
struct B { B(const int&); B(int&&) = delete; };
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, std::ranges::in_in_result<int, int>>);
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, std::ranges::in_in_result<int, int>&>);
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, const std::ranges::in_in_result<int, int>>);
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, const std::ranges::in_in_result<int, int>&>);

struct C { C(int&); };
static_assert(!std::is_constructible_v<std::ranges::in_in_result<C, C>, std::ranges::in_in_result<int, int>&>);
```
(Here I'm using `!is_constructible` to prove that there's not just "not an implicit ctor", but //also// not an explicit ctor //either//.)


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:49
+  }
+}
----------------
I'd also like to see tests that the conversion operator is unconditionally `noexcept(false)`.
(No conditional-explicit, no 4x overloads for perfect forwarding, no conditional noexcept... These result types are strangely old-school, compared to most Ranges stuff.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116278