[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#4124996 <https://reviews.llvm.org/D143914#4124996>, @rupprecht wrote:

> Still chugging away, looks like this is going to be at least 50 breakages. Most of them are trivial to fix (e.g. `return std::make_tuple()` in a method returning `std::pair` should be `return std::make_pair()`). There are a few that are a little surprising, such as this one which doesn't seem related to tuple compatibility:
>
>   void func() {
>     std::map<unsigned long, std::string> m;
>   
>     // OK
>     std::vector<std::pair<unsigned long, std::string>> v1(m.begin(), m.end());
>   
>     // OK before, but now: error: no matching constructor for initialization
>     std::vector<
>         std::pair<const unsigned long, std::reference_wrapper<std::string>>>
>         v2(m.begin(), m.end());
>   }

This seems to be the relevant part:

  include/c++/v1/vector:427:55: note: candidate template ignored: requirement 'is_constructible<std::pair<const unsigned long, std::reference_wrapper<std::string>>, std::pair<const unsigned long, std::string> &>::value' was not satisfied [with _ForwardIterator = std::__map_iterator<std::__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>]
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_ForwardIterator __first, _ForwardIterator __last);
                                                        ^

It basically boils down to this:

  #include <map>
  #include <string>
  #include <type_traits>
  #include <utility>
  #include <vector>
  
  int main(int, char**) {
    std::map<unsigned long, std::string> m;
  
    // OK
    using Pair1 = std::pair<unsigned long, std::string>;
    std::vector<Pair1> v1(m.begin(), m.end());
  
    // OK before, but not anymore
    using Pair2 = std::pair<const unsigned long, std::reference_wrapper<std::string>>;
    static_assert(std::is_constructible_v<Pair2, std::pair<const unsigned long, std::string> &>);
  }

This used to be accepted by libc++ but isn't anymore. This was never accepted by other standard libraries though: https://godbolt.org/z/c4svx1YTG.

Incidentally, checking for `static_assert(std::is_constructible_v<Pair2, std::pair<const unsigned long, std::string> const&>);` instead (notice I made the right hand side a `const&`) never works, on all stdlib implementations: https://godbolt.org/z/M8odqfvKM.

Hence, I'd argue this is somewhat brittle and I think this is both a corner case and also something that I'd be fine with breaking. Thoughts?

So, for the next steps:

1. I want to get the `subrange` `operator= const` vs `operator=` situation understood
2. I got back our internal build results and I did see a few failures similar to what @rupprecht saw. I think this will bite a few users, however the fix in most cases should be to use  `-std=c++2b` and the code will work out of the box. None of these failures seemed too hard to fix.



================
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.
+    {
----------------
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=`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143914