[libc++] Implement P1951, default arguments for pair's forwarding constructor

[Mordante]

LGTM, provided it passes the build. Please update the modification date in libcxx/docs/Status/Cxx2b.rst, which I usually also forget about ;-)
There was an issue with Buildkite, can you reupload the diff to start the build again?

[• Quuxplusone]

libcxx/test/std/utilities/utility/pairs/pairs.pair/U_V.pass.cpp

124

I'd like to see some tests for:

• SFINAE-friendliness. template<class T> void foo() -> decltype(std::pair<T, T>({}, {})) I guess should be SFINAE-friendly.
• CTAD, even though AIUI the class template arguments will never be deducible. Still, I guess void foo(auto t) -> decltype(std::pair({}, t)) should be SFINAE-friendly.
• Both brace-init and parens-init: std::pair<T, U>({}, {}) and std::pair<T, U>{{}, {}}.
• SFINAE-friendliness of something like this, i.e. we didn't mess up the conditional-explicit-ness of the constructors involved. (I may not have thought this one out sufficiently. pair's constructor overload set is very confusing.)

struct ExplicitBraceT { explicit ExplicitBraceT(); };
void f(std::pair<ExplicitBraceT, ExplicitBraceT>);  // #1
void f(std::pair<BraceInit, BraceInit>);  // #2
int main() { f({{}, {}}); }  // unambiguously calls #2, right?

Btw, there's precedent for this kind of "in standards greater than X, or _LIBCPP_VERSION" test, here: libcxx/test/std/utilities/utility/forward/{move,forward}.pass.cpp

[ldionne]

Please update the modification date in libcxx/docs/Status/Cxx2b.rst, which I usually also forget about ;-)

Is it really relevant to provide a modification date at all? I mean, it doesn't really add anything to the documentation IMO, and it's an obvious trap to forget to update it. IMO we should just remove those. Let's move this to D109087.

[ldionne]

Address review comments.

[ldionne]

libcxx/test/std/utilities/utility/pairs/pairs.pair/U_V.pass.cpp

124

SFINAE-friendliness.

Done.

CTAD, even though AIUI the class template arguments will never be deducible.

Done.

Both brace-init and parens-init: std::pair<T, U>({}, {}) and std::pair<T, U>{{}, {}}.

Added.

SFINAE-friendliness of something like this

Done.

Btw, there's precedent for this kind of "in standards greater than X [...]

I much prefer the way I've done it here - unless I misunderstand something, in libcxx/test/std/utilities/utility/forward/forward.pass.cpp, they duplicate the testing code just to account for the special case C++ == 11 && libc++.

[• Quuxplusone]

Looks like some GCC/Clang implementation divergence in re explicit_vs_implicit_brace_init({{}, {}}). I don't know who's correct.
Anyway, LGTM.

(Mild surprise that we're backporting this still-only-tentatively-C++23 change all the way back to C++11, but I have no better suggestion.)

libcxx/test/std/utilities/utility/pairs/pairs.pair/U_V.pass.cpp

120

(void)p; on its own line?

[ldionne]

In D109066#2982889, @Quuxplusone wrote:

Looks like some GCC/Clang implementation divergence in re explicit_vs_implicit_brace_init({{}, {}}). I don't know who's correct.
Anyway, LGTM.

Thanks. I'll look into the failure, I'll probably need to file a bug report and see who's right.

(Mild surprise that we're backporting this still-only-tentatively-C++23 change all the way back to C++11, but I have no better suggestion.)

Yeah.. I thought about doing it strictly like I always fight for, however in this case this seems to be closer to a DR than anything else. I mean the code previously would work just as well, but it would make a copy instead by using the pair(T1 const&, T2 const&) ctor. So IMO this is in essence closer to a LWG issue being fixed than an actual feature. If someone disagrees, LMK and I'll look into enabling this only in C++23, like we do for features/papers.

[Mordante]

In D109066#2983072, @ldionne wrote:

In D109066#2982889, @Quuxplusone wrote:

(Mild surprise that we're backporting this still-only-tentatively-C++23 change all the way back to C++11, but I have no better suggestion.)

Yeah.. I thought about doing it strictly like I always fight for, however in this case this seems to be closer to a DR than anything else. I mean the code previously would work just as well, but it would make a copy instead by using the pair(T1 const&, T2 const&) ctor. So IMO this is in essence closer to a LWG issue being fixed than an actual feature.

Actually I was surprised it wasn't a LWG issue. I didn't question the back-porting since I agree this is a good exception of our rule.

[• Quuxplusone]

I mean the code previously would work just as well, but it would make a copy instead by using the pair(T1 const&, T2 const&) ctor.

Well, now I'm even less sure I understand the procedural issues here. Surely if std::pair<T,U>({}, {}) was supposed to compile in C++20, then we should have some tests to prove that it compiles in C++20. It seems like those tests are the ones you're adding in U_V.pass.cpp; maybe we had no such tests before? But we're not running those tests pre-C++23 (except on libc++), so we still have no tests that test the expected C++20-and-earlier behavior? But on libc++ we don't even expect the expected behavior, anyway? 😛

[ldionne]

Address review comments and try to silence GCC potential bug.

[ldionne]

In D109066#2983114, @Quuxplusone wrote:

I mean the code previously would work just as well, but it would make a copy instead by using the pair(T1 const&, T2 const&) ctor.

Well, now I'm even less sure I understand the procedural issues here. [...]

Hmm, indeed, your point of view makes sense. Since the brace-init bits should always have worked, I added tests for those in all standard modes (except C++03, but whatever). Then, I added tests for just https://wg21.link/P1951 in C++23 and above (and on libc++ in all std modes as an extension).

Regarding the GCC issue, I filed a bug here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102247. I'll mark the test as XFAIL on GCC for now, and I'll revisit the test once I have more information.

[rsmith]

This is a breaking change. For example, this is valid code in C++20 and earlier but is now rejected by libc++:

void f(const std::pair<const std::string&, std::vector<int>> x) {}
void g() {
  f({"foo", {1, 2}});
}

The reason is that we used to call the pair(const string&, const vector<int>&) constructor (the converting constructor is not usable because we can't deduce from an initializer list), which created the string temporary as part of the call to f. We now call the pair(U&&, V&&) constructor, which creates a string temporary inside the pair constructor, and that temporary doesn't live long enough any more.

For what it's worth, I don't think it's super important to support this code: it's relying on a very fragile selection of pair constructor, and I think it's working by chance rather than by design, but this is reduced from real, valid code whose build broke after this patch landed. For the sake of conformance, perhaps we should do one of these things:

1. Get this change officially blessed as a DR, or
2. Add a !__reference_binds_to_temporary SFINAE check to the converting constructor in at least C++20 and before, or
3. Restrict this change to C++23 and later.

[rsmith]

Another example of valid C++20-and-before code (reduced from real code) that libc++ now rejects after this change:

void f() {
    const int n = 5;
    [] { std::pair<int, long>({1}, n); };
}

Here, n doesn't need to be captured if we call the pair(const int&, const long&) constructor, because the lvalue-to-rvalue conversion happens in the lambda. But if we call the pair(U&&, V&&) constructor (deducing V = int), then n does need to be captured and the build breaks.

[ldionne]

Thanks for the heads up, Richard. Let me take a look.

[ldionne]

I'm addressing this issue in https://reviews.llvm.org/D110347, see that review for details.

libcxx/docs/Status/Cxx2bPapers.csv

 "","","","","",""
 "`P0401R6 <https://wg21.link/P0401R6>`__","LWG","Providing size feedback in the Allocator interface","June 2021","",
 "`P0448R4 <https://wg21.link/P0448R4>`__","LWG","A strstream replacement using span<charT> as buffer","June 2021","",""
 "`P1132R8 <https://wg21.link/P1132R8>`__","LWG","out_ptr - a scalable output pointer abstraction","June 2021","",""
 "`P1328R1 <https://wg21.link/P1328R1>`__","LWG","Making std::type_info::operator== constexpr","June 2021","",""
 "`P1425R4 <https://wg21.link/P1425R4>`__","LWG","Iterators pair constructors for stack and queue","June 2021","",""
 "`P1518R2 <https://wg21.link/P1518R2>`__","LWG","Stop overconstraining allocators in container deduction guides","June 2021","|Complete|","13.0"
 "`P1659R3 <https://wg21.link/P1659R3>`__","LWG","starts_with and ends_with","June 2021","",""
-"`P1951R1 <https://wg21.link/P1951R1>`__","LWG","Default Arguments for pair Forwarding Constructor","June 2021","",""
+"`P1951R1 <https://wg21.link/P1951R1>`__","LWG","Default Arguments for pair Forwarding Constructor","June 2021","|Complete|","14.0"
 "`P1989R2 <https://wg21.link/P1989R2>`__","LWG","Range constructor for std::string_view","June 2021","",""
 "`P2136R3 <https://wg21.link/P2136R3>`__","LWG","invoke_r","June 2021","",""
 "`P2166R1 <https://wg21.link/P2166R1>`__","LWG","A Proposal to Prohibit std::basic_string and std::basic_string_view construction from nullptr","June 2021","|Complete|","13.0"
 "","","","","",""
 No newline at end of file

libcxx/include/__utility/pair.h

 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-
 #if defined(_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR)
 template <class, class>
 struct __non_trivially_copyable_base {
   _LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY
   __non_trivially_copyable_base() _NOEXCEPT {}
   _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
   __non_trivially_copyable_base(__non_trivially_copyable_base const&) _NOEXCEPT {}
 };
template<bool _Dummy = true, typename enable_if<
             _CheckArgsDep<_Dummy>::template __enable_implicit<_T1 const&, _T2 const&>()
     >::type* = nullptr>
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
     pair(_T1 const& __t1, _T2 const& __t2)
         _NOEXCEPT_(is_nothrow_copy_constructible<first_type>::value &&
                    is_nothrow_copy_constructible<second_type>::value)
         : first(__t1), second(__t2) {}
 
-    template<class _U1, class _U2, typename enable_if<
+    template<class _U1 = _T1, class _U2 = _T2, typename enable_if<
              _CheckArgs::template __enable_explicit<_U1, _U2>()
     >::type* = nullptr>
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
     explicit pair(_U1&& __u1, _U2&& __u2)
         _NOEXCEPT_((is_nothrow_constructible<first_type, _U1>::value &&
                     is_nothrow_constructible<second_type, _U2>::value))
         : first(_VSTD::forward<_U1>(__u1)), second(_VSTD::forward<_U2>(__u2)) {}
 
-    template<class _U1, class _U2, typename enable_if<
+    template<class _U1 = _T1, class _U2 = _T2, typename enable_if<
             _CheckArgs::template __enable_implicit<_U1, _U2>()
     >::type* = nullptr>
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
     pair(_U1&& __u1, _U2&& __u2)
         _NOEXCEPT_((is_nothrow_constructible<first_type, _U1>::value &&
                     is_nothrow_constructible<second_type, _U2>::value))
         : first(_VSTD::forward<_U1>(__u1)), second(_VSTD::forward<_U2>(__u2)) {}
 

libcxx/include/utility

struct pair
 
     T1 first;
     T2 second;
 
     pair(const pair&) = default;
     pair(pair&&) = default;
     explicit(see-below) constexpr pair();
     explicit(see-below) pair(const T1& x, const T2& y);                          // constexpr in C++14
-    template <class U, class V> explicit(see-below) pair(U&& x, V&& y);          // constexpr in C++14
+    template <class U = T1, class V = T2> explicit(see-below) pair(U&&, V&&);    // constexpr in C++14
     template <class U, class V> explicit(see-below) pair(const pair<U, V>& p);   // constexpr in C++14
     template <class U, class V> explicit(see-below) pair(pair<U, V>&& p);        // constexpr in C++14
     template <class... Args1, class... Args2>
         pair(piecewise_construct_t, tuple<Args1...> first_args,
              tuple<Args2...> second_args);                                       // constexpr in C++20
 
     template <class U, class V> pair& operator=(const pair<U, V>& p);            // constexpr in C++20
     pair& operator=(pair&& p) noexcept(is_nothrow_move_assignable<T1>::value &&

libcxx/test/std/utilities/utility/pairs/pairs.pair/U_V.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
 // <utility>
 
 // template <class T1, class T2> struct pair
 
-// template<class U, class V> pair(U&& x, V&& y);
-
+// template<class U = T1, class V = T2> pair(U&&, V&&);
 
 #include <utility>
-#include <memory>
 #include <cassert>
+#include <memory>
 
 #include "archetypes.h"
 #include "test_convertible.h"
 
 #include "test_macros.h"
 using namespace ImplicitTypes; // Get implicitly archetypes
 
 template <class T1, class T1Arg,
struct ExplicitT {
   int value;
 };
 
 struct ImplicitT {
   constexpr ImplicitT(int x) : value(x) {}
   int value;
 };
 
-
 int main(int, char**)
 {
     {
         typedef std::pair<std::unique_ptr<int>, short*> P;
         P p(std::unique_ptr<int>(new int(3)), nullptr);
         assert(*p.first == 3);
         assert(p.second == nullptr);
     }
#if TEST_STD_VER > 11
     }
     { // implicit constexpr test
         constexpr std::pair<ImplicitT, ImplicitT> p = {42, 43};
         static_assert(p.first.value == 42, "");
         static_assert(p.second.value == 43, "");
     }
 #endif
 
+    // Test support for http://wg21.link/P1951, default arguments for pair's constructor.
+    // Basically, this turns copies for brace initialization into moves. Note that libc++
+    // applies this in all standard modes.
+#if TEST_STD_VER > 20 || defined(_LIBCPP_VERSION)
+    {
+        struct TrackInit {
+            TrackInit() = default;
+            constexpr TrackInit(TrackInit const&) : wasCopyInit(true) { }
+            constexpr TrackInit(TrackInit&&) : wasMoveInit(true) { }
+            bool wasMoveInit = false;
+            bool wasCopyInit = false;
+        };
+
+        // Explicit constructor
+        {
+            {
+                std::pair<TrackInit, int> p({}, 3);
+                assert( p.first.wasMoveInit);
+                assert(!p.first.wasCopyInit);
+            }
+            {
+                std::pair<int, TrackInit> p(3, {});
+                assert( p.second.wasMoveInit);
+                assert(!p.second.wasCopyInit);
+            }
+        }
+
+        // Implicit constructor
+        {
+            {
+                std::pair<TrackInit, int> p = {{}, 3};
+                assert( p.first.wasMoveInit);
+                assert(!p.first.wasCopyInit);
+            }
+            {
+                std::pair<int, TrackInit> p = {3, {}};
+                assert( p.second.wasMoveInit);
+                assert(!p.second.wasCopyInit);
+            }
+        }
+    }
+#endif
+
     return 0;
 }

libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.brace-init.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// TODO: Revisit this test once we have more information
+//       with https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102247
+// XFAIL: gcc
+
+// <utility>
+
+// template <class T1, class T2> struct pair
+//
+// pair(const T1&, const T2&);
+// template<class U = T1, class V = T2> pair(U&&, V&&);
+
+// This test checks support for brace-initialization of pairs.
+
+#include <utility>
+#include <cassert>
+
+#include "test_macros.h"
+
+struct ExplicitT {
+  constexpr explicit ExplicitT(int x) : value(x) {}
+  constexpr explicit ExplicitT(ExplicitT const& o) : value(o.value) {}
+  int value;
+};
+
+struct ImplicitT {
+  constexpr ImplicitT(int x) : value(x) {}
+  constexpr ImplicitT(ImplicitT const& o) : value(o.value) {}
+  int value;
+};
+
+template <class T, class = decltype(std::pair<T, T>({}, {}))>
+constexpr bool can_construct_with_brace_init(int) { return true; }
+template <class T>
+constexpr bool can_construct_with_brace_init(...) { return false; }
+
+#if TEST_STD_VER >= 17 // CTAD isn't supported before C++17
+template <class T, class = decltype(std::pair(T{}, {}))>
+constexpr bool can_construct_with_ctad_brace_init(int) { return true; }
+template <class T>
+constexpr bool can_construct_with_ctad_brace_init(...) { return false; }
+#endif
+
+struct BraceInit { BraceInit() = default; };
+struct NoBraceInit { NoBraceInit(int); };
+struct ExplicitBraceInit { explicit ExplicitBraceInit() = default; };
+
+constexpr int explicit_vs_implicit_brace_init(std::pair<ExplicitBraceInit, ExplicitBraceInit>) { return 1; }
+constexpr int explicit_vs_implicit_brace_init(std::pair<BraceInit, BraceInit>) { return 2; }
+
+TEST_CONSTEXPR_CXX14 bool test() {
+    // Explicit constructor
+    {
+        std::pair<ExplicitT, BraceInit> p1(ExplicitT{42}, {});
+        assert(p1.first.value == 42);
+
+        std::pair<ExplicitT, BraceInit> p2{ExplicitT{42}, {}};
+        assert(p2.first.value == 42);
+    }
+    {
+        std::pair<BraceInit, ExplicitT> p1({}, ExplicitT{42});
+        assert(p1.second.value == 42);
+
+        std::pair<BraceInit, ExplicitT> p2{{}, ExplicitT{42}};
+        assert(p2.second.value == 42);
+    }
+    {
+        std::pair<BraceInit, BraceInit> p{{}, {}};
+        (void)p;
+    }
+
+    // Implicit constructor
+    {
+        std::pair<ImplicitT, BraceInit> p = {42, {}};
+        assert(p.first.value == 42);
+    }
+    {
+        std::pair<BraceInit, ImplicitT> p = {{}, 42};
+        assert(p.second.value == 42);
+    }
+    {
+        std::pair<BraceInit, BraceInit> p = {{}, {}};
+        (void)p;
+    }
+
+    // SFINAE-friendliness of some invalid cases
+    {
+        static_assert( can_construct_with_brace_init<BraceInit>(0), "");
+        static_assert(!can_construct_with_brace_init<NoBraceInit>(0), "");
+
+#if TEST_STD_VER >= 17
+        // CTAD with {} should never work, since we can't possibly deduce the types
+        static_assert(!can_construct_with_ctad_brace_init<BraceInit>(0), "");
+        static_assert(!can_construct_with_ctad_brace_init<int>(0), "");
+#endif
+    }
+
+    // Make sure there is no ambiguity between the explicit and the non-explicit constructors
+    {
+        assert(explicit_vs_implicit_brace_init({{}, {}}) == 2);
+    }
+
+    return true;
+}
+
+int main(int, char**) {
+    test();
+#if TEST_STD_VER > 11
+    static_assert(test(), "");
+#endif
+
+    return 0;
+}