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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__utility/pair.h:63
 
+#if _LIBCPP_STD_VER >= 23
+template <class _Tp>
----------------
ldionne wrote:
> 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.
I know the feeling of the rabbit hole :-/


================
Comment at: libcxx/include/__utility/pair.h:134
 
-    struct _CheckTupleLikeConstructor {
-        template <class _Tuple>
----------------
ldionne wrote:
> 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).
Sound fine to me, just wanted to make sure it's a deliberate choice. 


================
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
----------------
IIRC the style using an int is the preferred style. Feel free to ignore it when I'm wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143914