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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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>>) { };
----------------
ldionne wrote:
> 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).
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110347