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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked an inline comment as done.
ldionne added a comment.

In D143914#4123632 <https://reviews.llvm.org/D143914#4123632>, @Mordante wrote:

> I mainly glossed over the code. I like to do a review with a paper or is this the exact wording in the current WP?

This is the wording in the current draft, modulo the reference_constructs_from_temporary bits. I had to do a full survey of what we do implement and what we didn't implement in various standard modes to try and come up with this.



================
Comment at: libcxx/include/__utility/pair.h:63
 
+#if _LIBCPP_STD_VER >= 23
+template <class _Tp>
----------------
Mordante wrote:
> Does this patch complete a paper/LWG issue?
In itself, no. It is preparing the terrain for being able to resolve P2255. That was originally my goal, in fact, and then I had to go down the rabbit hole when I saw that our pair constructors and assignment operators were not conforming at all.


================
Comment at: libcxx/include/__utility/pair.h:134
 
-    struct _CheckTupleLikeConstructor {
-        template <class _Tuple>
----------------
Mordante wrote:
> Would it be hard to keep these removed parts optionally available when using C++ < 23?
> If so we could do our normal deprecation method.
> 
> For me it's not a blocker if it's too hard.
Our usual deprecation method is to write it in the release notes, however in this case I am not sure how helpful that would be -- I doubt a lot of users read it and this extension has been there so long that it is probably taken for granted. Unless it breaks an unreasonable amount of code, IMO we should just go for it, it's not worse than a bunch of other fixes we've done in the past.

The other downside with deprecating this is that we'd now have a behavior that works in C++11/C++14/C++17/C++20 but is deprecated, and a subtly different non-deprecated behavior in C++23. As a user, I think I would find this rather confusing.

Before deciding to add complexity around this, I'd like to see how bad this is in real world code bases -- I'm waiting on Google's feedback here and I also started internal builds `105406484&105406488&105406497` (for my own ref).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143914