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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/results.h:10-11
+
+#ifndef _LIBCPP___ALGORITHM_IN_IN_RESULT_H
+#define _LIBCPP___ALGORITHM_IN_IN_RESULT_H
+
----------------
Quuxplusone wrote:
> 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.
I see you want me to do more work :). I'll add one soon.


================
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);
----------------
Quuxplusone wrote:
> 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//.)
You can't cast to `std::ranges::in_in_result<double, int>`, because that would be narrowing `int` to `double` and `double` to `int`, which is not allowed here IIUC.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:49
+  }
+}
----------------
Quuxplusone wrote:
> 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.)
Do you want more than checking that all `const` and `&` possibilities are checked to be `noexcept(false)` or is that enough?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116278