[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/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:
> 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`.
It seems the only difference is that GCC disables conversion warnings: https://godbolt.org/z/TvfTsjP5d. The implementation itself is exactly the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116278