[libcxx-commits] [PATCH] D110347: [libc++] Do not enable P1951 before C++23, since it's a breaking change

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.brace-init.P1951.pass.cpp:20-28
+    // Example 1:
+    // Without P1951, we 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 creates the string temporary as part of the
+    // call to f. With P1951, we 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.
+    {
+        auto f = [](std::pair<const std::string&, std::vector<int>>) { };
----------------
Quuxplusone wrote:
> 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?
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110347