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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Quuxplusone wrote:
> 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?
This is still unaddressed, right?
I think it would suffice to make an `algorithms.results/deduct.compile.pass.cpp` covering all the types, on the model of your existing `no_unique_address.compile.pass.cpp`.
```
ASSERT_SAME_TYPE(decltype(std::ranges::in_in_result(1, 'x')), std::ranges::in_in_result<int, char>);
ASSERT_SAME_TYPE(decltype(std::ranges::in_in_out_result(1, 'x', 1.f)), std::ranges::in_in_result<int, char, float>);
```
etc.


================
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);
----------------
philnik wrote:
> Quuxplusone wrote:
> > 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.)
> It's the same as in D116278.
Ah; consider removing it from both places here or in a followup. :)


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:34-41
+struct B {
+  B(const int&);
+  B(int&&);
+};
+static_assert(std::is_constructible_v<std::ranges::in_in_out_result<B, B, B>, std::ranges::in_in_out_result<int, int, int>>);
+static_assert(std::is_constructible_v<std::ranges::in_in_out_result<B, B, B>, std::ranges::in_in_out_result<int, int, int>&>);
+static_assert(std::is_constructible_v<std::ranges::in_in_out_result<B, B, B>, const std::ranges::in_in_out_result<int, int, int>>);
----------------
FWIW, I think lines 35-36 should just be `B(int);` — I don't see any realistic way for the behavior to differ between these two overload sets.
Line 29 could use a comment like `// conversion is not implicit` (to explain why line 31 is false not true).


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:98
+    assert(res1.in1.get() == 1);
+    [[maybe_unused]] auto res2 = static_cast<std::ranges::in_in_out_result<MoveOnly, int, int>>(std::move(res1));
+    assert(res1.in1.get() == 0);
----------------
You can remove `[[maybe_unused]]` now that the variable //is// used. (In general, `[[maybe_unused]]` hides missing assertions, so I'd advise, as a first approximation, never using it.)


================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:33-35
+static_assert(sizeof(std::ranges::in_in_out_result<char, Empty, Empty>) == sizeof(char) + sizeof(Empty));
+static_assert(sizeof(std::ranges::in_in_out_result<Empty, char, Empty>) == sizeof(char) + sizeof(Empty));
+static_assert(sizeof(std::ranges::in_in_out_result<Empty, Empty, char>) == sizeof(char) + sizeof(Empty));
----------------
I predict this getting cumbersome to read; I'd still rather see `== 2` here, and ditto on line 28: basically pre-constant-fold everything we know to be a constant `1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117512