[libcxx-commits] [PATCH] D143914: [libc++] Clean up pair's constructors and assignment operators

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp:57
+
+    // Make sure it doesn't work for `ranges::subrange`, since it is called out explicitly.
+    {
----------------
ldionne wrote:
> huixie90 wrote:
> > tcanens wrote:
> > > ldionne wrote:
> > > > @tcanens @huixie90 
> > > > 
> > > > This behavior seems to be inconsistent with the non-const-qualified `operator=` from `pair-like`. For `operator=(PairLike)` (not `const`), it seems like we go through `subrange`'s `operator pair-like()`, which makes this work. But with `operator=(pair-like) const`, it looks like this conversion doesn't trigger and we end up not being able to perform the assignment. Does that match the LWG design intent?
> > > It doesn't trigger because `ConstAssignable` is not convertible from `int*`? Are the const/non-const cases actually different?
> > > 
> > > IIRC the constraint is there to prevent bypassing the slicing check on subrange's `operator pair-like`.
> > speaking of `subrange`, I think it is worth adding more tests such as
> > ```
> > Pair p = subrange1;
> > Pair p{subrange1};
> > auto p = static_cast<Pair>(subrange);
> > ```
> > where the pair's constructor and subrange's conversion operator both can work.
> > (apparently subrange's conversion operator is more constrained than pair's constructor but I think that does not change anything?)
> @tcanens 
> 
> > It doesn't trigger because ConstAssignable is not convertible from int*
> 
> Ah, yes, obviously that's *one* problem. But even after fixing it, the behavior for const and non-const is different AFAICT:
> 
> ```
> // const assignment
> struct ConstAssignable {
>   mutable int* ptr = nullptr;
>   ConstAssignable() = default;
>   constexpr ConstAssignable(int* p) : ptr(p) { } // enable `subrange::operator pair-like`
>   constexpr ConstAssignable const& operator=(int* p) const { ptr = p; return *this; }
> };
> 
> static_assert( std::is_assignable_v<std::pair<ConstAssignable, ConstAssignable> const&, std::tuple<int*, int*>>); // test the test
> static_assert(!std::is_assignable_v<std::pair<ConstAssignable, ConstAssignable> const&, std::ranges::subrange<int*>>);
> 
> // non-const assignment
> struct Assignable {
>   int* ptr = nullptr;
>   Assignable() = default;
>   constexpr Assignable(int* p) : ptr(p) { } // enable `subrange::operator pair-like`
>   constexpr Assignable& operator=(int* p) { ptr = p; return *this; }
> };
> int data[] = {1, 2, 3, 4, 5};
> std::ranges::subrange<int*> a(data);
> std::pair<Assignable, Assignable> p;
> p = a; // that's fine
> ```
> 
> Did I miss something else here? There must be something else going on that causes the `operator pair-like` for `std::subrange` to not trigger when the elements are const-assignable but not "just" assignable. Honestly, I am left wondering whether a `const` `operator=` triggers the same sequence of implicit conversions than the non-`const` `operator=`.
> 
The difference is that `ConstAssignable` isn't const-assignable from `ConstAssignable` (but `Assignable` is assignable from `Assignable`).

`operator pair-like` converts the subrange to a `std::pair<ConstAssignable, ConstAssignable>` (or std::pair<Assignable, Assignable>`) and the assignment is performed from that, so you need const-assignment from `ConstAssignable`, not `int*`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143914