[libcxx-commits] [PATCH] D117512: [libc++][ranges] Add ranges::in_in_out_result

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

LGTM % test comments, but I'll request changes anyway.



================
Comment at: libcxx/include/__algorithm/in_in_out_result.h:28-32
+  template <class _II1, class _II2, class _OO1>
+  requires convertible_to<const _I1&, _II1> && convertible_to<const _I2&, _II2> && convertible_to<const _O1&, _OO1>
+      _LIBCPP_HIDE_FROM_ABI constexpr operator in_in_out_result<_II1, _II2, _OO1>() const& {
+    return {in1, in2, out};
+  }
----------------
Some whitespace issues here. Ditto below.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I don't see a CTAD test. How are we doing CTAD tests for these types? `deduct.pass.cpp`, or dump it in here, or what?


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:79
+    assert(res.out == 1.f);
+    std::ranges::in_in_out_result<ConstructibleFrom<int>, ConstructibleFrom<double>, ConstructibleFrom<float>> res2 = res;
+    assert(res2.in1.content == 10);
----------------
I don't think this buys you anything over a simple
```
std::ranges::in_in_out_result<long, float, int> res2 = res;
assert(res2.in1 == 10L);
~~~
```
(Also, moot point now, but `ConstructibleFrom` would be the wrong name for something that's implicitly //convertible// from; `ConvertibleFrom` would have been a better name.)


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:84-89
+  {
+    std::ranges::in_in_out_result<MoveOnly, int, int> res{MoveOnly{}, 0, 0};
+    assert(res.in1.get() == 1);
+    [[maybe_unused]] auto res2 = static_cast<std::ranges::in_in_out_result<MoveOnly, int, int>>(std::move(res));
+    assert(res.in1.get() == 0);
+  }
----------------
Since it was originally the constraint on `_O1` that was missing, I think it makes sense to test all of the positions here. This can be as simple as
```
  {
    std::ranges::in_in_out_result<MoveOnly, int, int> res{MoveOnly(1), 0, 0};
    auto res2 = std::ranges::in_in_out_result<MoveOnly, int, int>(std::move(res));
    assert(res.in1.get() == 0);  // it was moved-from
    assert(res2.in1.get() == 1);
    static_assert( std::is_constructible_v<std::ranges::in_in_out_result<MoveOnly, MoveOnly, MoveOnly>, std::ranges::in_in_out_result<int, int, int>&>);
    static_assert( std::is_move_constructible_v<std::ranges::in_in_out_result<MoveOnly, int, int>>);
    static_assert( std::is_move_constructible_v<std::ranges::in_in_out_result<int, MoveOnly, int>>);
    static_assert( std::is_move_constructible_v<std::ranges::in_in_out_result<int, int, MoveOnly>>);
    // Non-copyability of any element makes the whole thing non-copyable.
    static_assert(!std::is_copy_constructible_v<std::ranges::in_in_out_result<MoveOnly, int, int>>);
    static_assert(!std::is_copy_constructible_v<std::ranges::in_in_out_result<int, MoveOnly, int>>);
    static_assert(!std::is_copy_constructible_v<std::ranges::in_in_out_result<int, int, MoveOnly>>);
  }
```


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:90-93
+  auto [in1, in2, out] = std::ranges::in_in_out_result<int, int, int>{1, 2, 3};
+  assert(in1 == 1);
+  assert(in2 == 2);
+  assert(out == 3);
----------------
Please wrap these four lines in `{ }` too, for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117512