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

[Jordan Rupprecht via Phabricator via libcxx-commits]

rupprecht added a comment.

The total impact for us is ~400 files, but very trivial in most cases, and usually only a couple impacted lines per file.

AFAICT, the breakage is entirely internal. I haven't found a single breakage in any external code. There could be any number of reasons to explain this, but generally speaking I would expect this to not have a broad impact -- the large breakage we see is probably the outlier.

I don't have a personal opinion whether this is good to land, as I can see arguments either way. I'd still like some time to continue cleaning things up -- 2-3 weeks is likely good enough to get most if not all of it done.

In D143914#4136705 <https://reviews.llvm.org/D143914#4136705>, @ldionne wrote:

> 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?

I'm fine accepting this outcome, but OOC, do you know why it's (erroneously) being accepted prior to this patch? Is it doing an implicit conversion roundtrip through something like `pair<T, U>` -> `tuple<T, U>` -> `pair<const T, std::reference_wrapper<U>>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143914