[libc++] Do not enable P1951 before C++23, since it's a breaking change

[ldionne]

Summary

In reaction to the issues raised by Richard in https://llvm.org/D109066,
this commit does not apply P1951 as a DR in previous standard modes,
since it breaks valid code.

I do believe it should be applied as a DR, however ideally we'd get some
sort of statement from the Committee to this effect (and all implementations
would behave consistently). In the meantime, only implement P1951 starting
with C++23 -- we can always come back and apply it as a DR if that's what
the Committee says.

[• Quuxplusone]

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

21–29

Well, this compiles either way, right? So to actually test the situation, it would be better to actually use the string's contents inside f. Or write a contrived toy version like

struct A {
    int *p_;
    A(int *p) : p_(p) { *p_ += 1; }
    A(const A& a) : p_(a.p_) { *p_ += 1; }
    ~A() { *p_ -= 1; }
};
int i = 0;
auto f = [&](std::pair<int, const A&>) { assert(i >= 1); };
f({42, &i});

https://godbolt.org/z/39q13bx41
Looks like libstdc++ did it as a DR (or has some other bug), and MSVC hasn't implemented it at all yet?

35–38

This test seems fine, since the symptom is "fail to compile."
Weirdly, libstdc++ is OK with this test; which is why I wondered above if they had some other bug causing the first test to fail.

[ldionne]

Address review comments.

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

21–29

No, it doesn't compile:

In file included from <..>/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.brace-init.P1951.pass.cpp:15:
In file included from <..>/build/default/include/c++/v1/string:520:
In file included from <..>/build/default/include/c++/v1/__functional_base:26:
In file included from <..>/build/default/include/c++/v1/utility:225:
<..>/build/default/include/c++/v1/__utility/pair.h:189:17: error: reference member 'first' binds to a temporary object whose lifetime would be shorter than the lifetime of the constructed object
        : first(_VSTD::forward<_U1>(__u1)), second(_VSTD::forward<_U2>(__u2)) {}
                ^~~~~~~~~~~~~~~~~~~~~~~~~
<..>/build/default/include/c++/v1/__config:755:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_ABI_NAMESPACE
              ^
<..>/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.brace-init.P1951.pass.cpp:27:11: note: in instantiation of function template specialization 'std::pair<const std::string &, std::vector<int>>::pair<char const (&)[4], std::vector<int>, nullptr>' requested here
        f({"foo", {1, 2}});
          ^
<..>/build/default/include/c++/v1/__utility/pair.h:48:9: note: reference member declared here
    _T1 first;
        ^

I guess it might just be Clang being helpful, though, so I agree we could make the test more robust (also GCC doesn't produce a build failure AFAICT).

[• Quuxplusone]

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

21–29

FWIW, yeah, I noticed Clang giving that error on Godbolt. Making it a hard error, instead of a warning, is... aggressive... if the error message is not mandated by the Standard; so a priori I'd guess it must be mandated by the Standard.
But no other compiler gives that error message, and the code is certainly reasonable in practice (as long as the caller never tries to use the reference that is now dangling). So maybe this is Clang being helpful in a slightly extralegal, "vigilante" sense.

[ldionne]

Re-upload -- I accidentally removed the fix before uploading last revision. This should be good to go, @Quuxplusone can you please have a look?

[• Quuxplusone]

I'm happy if buildkite is happy. My comment about lines 116+117 is worth addressing one way or the other (either complicate the ctors, or remove the redundant assertions), but I don't really care which way.

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

105–109

FWIW, the paired assertions (e.g. lines 116+117) seem like overkill, since right now obviously it's impossible ever to have wasCopyInit && wasMoveInit. I wonder if you meant to do, like,

constexpr TrackInit(TrackInit const& rhs) : wasCopyInit(true), wasMoveInit(rhs.wasMoveInit) { }
constexpr TrackInit(TrackInit&& rhs) : wasCopyInit(rhs.wasCopyInit), wasMoveInit(true) { }

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

35–36

LGTM, although of course you could replace vector<int> with int and/or {42,43} with {} if you wanted to simplify it a bit.

[ldionne]

Address comments.

[ldionne]

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

35–36

(I think this comment does not appear on the right line).

Actually, if you do that, Clang starts diagnosing that we're binding a temporary beyond its lifetime. That *sounds* like a false-positive to me.

[ldionne]

Will ship once BuildKite is happy.

libcxx/include/__utility/pair.h

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 = _T1, class _U2 = _T2, typename enable_if<
-             _CheckArgs::template __enable_explicit<_U1, _U2>()
-    >::type* = nullptr>
+    template <
+#if _LIBCPP_STD_VER > 20 // http://wg21.link/P1951
+        class _U1 = _T1, class _U2 = _T2,
+#else
+        class _U1, class _U2,
+#endif
+        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 = _T1, class _U2 = _T2, typename enable_if<
-            _CheckArgs::template __enable_implicit<_U1, _U2>()
-    >::type* = nullptr>
+    template <
+#if _LIBCPP_STD_VER > 20 // http://wg21.link/P1951
+        class _U1 = _T1, class _U2 = _T2,
+#else
+        class _U1, class _U2,
+#endif
+        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)) {}
 
     template<class _U1, class _U2, typename enable_if<
             _CheckArgs::template __enable_explicit<_U1 const&, _U2 const&>()

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

#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)
+    // Basically, this turns copies for brace initialization into moves.
+#if TEST_STD_VER > 20
     {
         struct TrackInit {
             TrackInit() = default;
-            constexpr TrackInit(TrackInit const&) : wasCopyInit(true) { }
-            constexpr TrackInit(TrackInit&&) : wasMoveInit(true) { }
+            constexpr TrackInit(TrackInit const& other) : wasMoveInit(other.wasMoveInit), wasCopyInit(true) { }
+            constexpr TrackInit(TrackInit&& other) : wasMoveInit(true), wasCopyInit(other.wasCopyInit) { }
             bool wasMoveInit = false;
             bool wasCopyInit = false;
         };
 
         // Explicit constructor
         {
             {
                 std::pair<TrackInit, int> p({}, 3);
                 assert( p.first.wasMoveInit);
                 assert(!p.first.wasCopyInit);
#if TEST_STD_VER > 20
             }
             {
                 std::pair<int, TrackInit> p = {3, {}};
                 assert( p.second.wasMoveInit);
                 assert(!p.second.wasCopyInit);
             }
         }
     }
-#endif
+#endif // TEST_STD_VER > 20
 
     return 0;
 }

libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.brace-init.P1951.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
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: c++11 || c++14 || c++17 || c++20
+
+// This test makes sure that we don't apply P1951 before C++23, since that is
+// a breaking change. The examples in this test are taken from Richard Smith's
+// comments on https://llvm.org/D109066.
+
+#include <cassert>
+#include <utility>
+#include <vector>
+
+struct A {
+    int *p_;
+    A(int *p) : p_(p) { *p_ += 1; }
+    A(const A& a) : p_(a.p_) { *p_ += 1; }
+    ~A() { *p_ -= 1; }
+};
+
+int main(int, char**) {
+    // Example 1:
+    // Without P1951, we call the `pair(int, const A&)` constructor (the converting constructor is not usable because
+    // we can't deduce from an initializer list), which creates the A temporary as part of the call to f. With P1951,
+    // we call the `pair(U&&, V&&)` constructor, which creates a A temporary inside the pair constructor, and that
+    // temporary doesn't live long enough any more.
+    {
+        int i = 0;
+        auto f = [&](std::pair<std::vector<int>, const A&>) { assert(i >= 1); };
+        f({{42, 43}, &i});
+    }
+
+    // Example 2:
+    // 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.
+    {
+        const int n = 5;
+        (void) []{ std::pair<int, long>({1}, n); };
+    }
+
+    return 0;
+}