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

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

Quuxplusone added inline comments.


================
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);
----------------
philnik wrote:
> 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.
libstdc++ thinks `in_in_result<int, double>` is convertible to `in_in_result<double, int>` https://godbolt.org/z/WaeTMq6T5 and it makes intuitive sense to me that it should be convertible, because `int` is convertible to `double` and vice versa.
But Microsoft STL produces the SFINAE-unfriendly narrowing diagnostic you were worried about: https://godbolt.org/z/qTqsYKn9c
Do you feel like digging into libstdc++ and figuring out whether they're doing something blatantly problematic to make this work? libstdc++'s behavior certainly //feels// nicer to me: whenever the conversion operator //exists//, it ought to //compile//. If we followed MSVC's lead here, we'd basically be lying to `std::is_convertible_v`.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp:49
+  }
+}
----------------
philnik wrote:
> 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?
What you have below seems to me like enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116278