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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 4 inline comments as done.
ldionne added a comment.

In D143914#4145669 <https://reviews.llvm.org/D143914#4145669>, @jyknight wrote:

> I wonder if it'd make sense to keep the extension in earlier standards modes, but modified to have the C++23 requirements.

Instead, here's what I suggest: I refactored the extension so that it would be self-contained in a separate set of constructors and assignment operators. That way, it's mostly out of the way for implementing the actual standard-specified constructors and assignment operators.

As it stands, this patch should not be a source break in most cases. There might be a few tiny cases where the new conversions will behave unlike the old conversions, but it should more or less work out of the box. I suggest that we then mark these extensions as deprecated with a message telling users to move to C++23 to get this behavior, and then remove the extensions in a future release. That will provide a better experience for folks.



================
Comment at: libcxx/include/__utility/pair.h:321-322
+        is_assignable<first_type&, _U1 const&>::value &&
+        is_assignable<second_type&, _U2 const&>::value
+    >* = nullptr>
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
----------------
Mordante wrote:
> IIRC the style using an int is the preferred style. Feel free to ignore it when I'm wrong.
We use `nullptr` elsewhere in `pair.h`, so I'll stick to that.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv.pass.cpp:80
+    static_assert(!std::is_assignable_v<std::pair<long, std::string>&, std::tuple<long, void*>>); // second not convertible
+    static_assert( std::is_assignable_v<std::pair<long, std::string>&, std::tuple<long, std::string>>); // works (test the test)
+  }
----------------
tcanens wrote:
> ldionne wrote:
> > @huixie90 @tcanens 
> > 
> > While implementing this, I found out that stuff like this would compile just fine:
> > 
> > ```
> > std::pair<long, std::string> p;
> > p = std::tuple<long, float>{};
> > ```
> > 
> > This is because `std::string` can be assigned-to from `float`. While this has nothing to do with `pair` specifically, this seems completely bonkers to me -- this is almost Javascript bad. I'm curious to know whether that has been discussed and acknowledged?
> See https://cplusplus.github.io/LWG/issue3311 and the duplicate.
> 
> 
Thanks for the pointer!


================
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.
+    {
----------------
tcanens wrote:
> 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*`.
Ah, of course, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143914