On Sat, 13 Jan 2024, Jonathan Wakely wrote:
> On Sat, 13 Jan 2024 at 00:06, Patrick Palka <ppa...@redhat.com> wrote:
> >
> > On Fri, 12 Jan 2024, Jonathan Wakely wrote:
> >
> > > On Fri, 12 Jan 2024 at 18:33, Patrick Palka <ppa...@redhat.com> wrote:
> > > >
> > > > On Fri, 12 Jan 2024, Jonathan Wakely wrote:
> > > >
> > > > > On Fri, 12 Jan 2024 at 17:55, Patrick Palka <ppa...@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, 11 Jan 2024, Jonathan Wakely wrote:
> > > > > >
> > > > > > > I'd like to commit this to trunk for GCC 14. Please take a look.
> > > > > > >
> > > > > > > -- >8 --
> > > > > > >
> > > > > > > This is the last part of PR libstdc++/108822 implementing
> > > > > > > P2255R2, which
> > > > > > > makes it ill-formed to create a std::tuple that would bind a
> > > > > > > reference
> > > > > > > to a temporary.
> > > > > > >
> > > > > > > The dangling checks are implemented as deleted constructors for
> > > > > > > C++20
> > > > > > > and higher, and as Debug Mode static assertions in the
> > > > > > > constructor body
> > > > > > > for older standards. This is similar to the
> > > > > > > r13-6084-g916ce577ad109b
> > > > > > > changes for std::pair.
> > > > > > >
> > > > > > > As part of this change, I've reimplemented most of std::tuple for
> > > > > > > C++20,
> > > > > > > making use of concepts to replace the enable_if constraints, and
> > > > > > > using
> > > > > > > conditional explicit to avoid duplicating most constructors. We
> > > > > > > could
> > > > > > > use conditional explicit for the C++11 implementation too (with
> > > > > > > pragmas
> > > > > > > to disables the -Wc++17-extensions warnings), but that should be
> > > > > > > done as
> > > > > > > a stage 1 change for GCC 15 rather than now.
> > > > > > >
> > > > > > > The partial specialization for std::tuple<T1, T2> is no longer
> > > > > > > used for
> > > > > > > C++20 (or more precisely, for a C++20 compiler that supports
> > > > > > > concepts
> > > > > > > and conditional explicit). The additional constructors and
> > > > > > > assignment
> > > > > > > operators that take std::pair arguments have been added to the
> > > > > > > C++20
> > > > > > > implementation of the primary template, with
> > > > > > > sizeof...(_Elements)==2
> > > > > > > constraints. This avoids reimplementing all the other
> > > > > > > constructors in
> > > > > > > the std::tuple<T1, T2> partial specialization to use concepts.
> > > > > > > This way
> > > > > > > we avoid four implementations of every constructor and only have
> > > > > > > three!
> > > > > > > (The primary template has an implementation of each constructor
> > > > > > > for
> > > > > > > C++11 and another for C++20, and the tuple<T1,T2> specialization
> > > > > > > has an
> > > > > > > implementation of each for C++11, so that's three for each
> > > > > > > constructor.)
> > > > > > >
> > > > > > > In order to make the constraints more efficient on the C++20
> > > > > > > version of
> > > > > > > the default constructor I've also added a variable template for
> > > > > > > the
> > > > > > > __is_implicitly_default_constructible trait, implemented using
> > > > > > > concepts.
> > > > > > >
> > > > > > > libstdc++-v3/ChangeLog:
> > > > > > >
> > > > > > > PR libstdc++/108822
> > > > > > > * include/std/tuple (tuple): Add checks for dangling
> > > > > > > references.
> > > > > > > Reimplement constraints and constant expressions using C++20
> > > > > > > features.
> > > > > > > * include/std/type_traits [C++20]
> > > > > > > (__is_implicitly_default_constructible_v): Define.
> > > > > > > (__is_implicitly_default_constructible): Use variable
> > > > > > > template.
> > > > > > > * testsuite/20_util/tuple/dangling_ref.cc: New test.
> > > > > > > ---
> > > > > > > libstdc++-v3/include/std/tuple | 1021
> > > > > > > ++++++++++++-----
> > > > > > > libstdc++-v3/include/std/type_traits | 11 +
> > > > > > > .../testsuite/20_util/tuple/dangling_ref.cc | 105 ++
> > > > > > > 3 files changed, 841 insertions(+), 296 deletions(-)
> > > > > > > create mode 100644
> > > > > > > libstdc++-v3/testsuite/20_util/tuple/dangling_ref.cc
> > > > > > >
> > > > > > > diff --git a/libstdc++-v3/include/std/tuple
> > > > > > > b/libstdc++-v3/include/std/tuple
> > > > > > > index 50e11843757..cd05b638923 100644
> > > > > > > --- a/libstdc++-v3/include/std/tuple
> > > > > > > +++ b/libstdc++-v3/include/std/tuple
> > > > > > > @@ -752,11 +752,467 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > > > > > template<typename... _Elements>
> > > > > > > class tuple : public _Tuple_impl<0, _Elements...>
> > > > > > > {
> > > > > > > - typedef _Tuple_impl<0, _Elements...> _Inherited;
> > > > > > > + using _Inherited = _Tuple_impl<0, _Elements...>;
> > > > > > >
> > > > > > > template<bool _Cond>
> > > > > > > using _TCC = _TupleConstraints<_Cond, _Elements...>;
> > > > > >
> > > > > > I guess this should be moved into the #else branch if it's not used
> > > > > > in
> > > > > > the new impl.
> > > > >
> > > > > Ah yes, I left them there until I was sure I wouldn't need them ...
> > > > > then didn't move them when I didn't need them.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > +#if __cpp_concepts && __cpp_conditional_explicit // >= C++20
> > > > > > > + template<typename... _UTypes>
> > > > > > > + static consteval bool
> > > > > > > + __constructible()
> > > > > > > + {
> > > > > > > + if constexpr (sizeof...(_UTypes) == sizeof...(_Elements))
> > > > > > > + return (is_constructible_v<_Elements, _UTypes> && ...);
> > > > > >
> > > > > > IIUC this (and all the other new constraints) won't short-circuit
> > > > > > like
> > > > > > the old versions do :/ Not sure how much that matters?
> > > > >
> > > > > Yeah, I thought about that, but we have efficient built-ins for these
> > > > > traits now, so I think it's probably OK?
> > > >
> > > > Performance wise agreed, though I suppose removing the short circuiting
> > > > could break existing (though not necessarily valid) code that relied
> > > > on it to prevent an ill-formed template instantiation. It seems
> > > > the standard https://eel.is/c++draft/tuple uses conjunction_v in some
> > > > constraints, and fold-expressions in others, implying short circuiting
> > > > in some cases but not others?
> > > >
> > > > >
> > > > > If not we could go back to sharing the _TupleConstraints
> > > > > implementations.
> > > >
> > > > IMHO I'd be more comfortable with that.
> > >
> > > Here's an incremental diff to make that change:
> > >
> > > --- a/libstdc++-v3/include/std/tuple
> > > +++ b/libstdc++-v3/include/std/tuple
> > > @@ -763,7 +763,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > __constructible()
> > > {
> > > if constexpr (sizeof...(_UTypes) == sizeof...(_Elements))
> > > - return (is_constructible_v<_Elements, _UTypes> && ...);
> > > + return __and_v<is_constructible<_Elements, _UTypes>...>;
> > > else
> > > return false;
> > > }
> > > @@ -773,7 +773,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > __nothrow_constructible()
> > > {
> > > if constexpr (sizeof...(_UTypes) == sizeof...(_Elements))
> > > - return (is_nothrow_constructible_v<_Elements, _UTypes> &&
> > > ...);
> > > + return __and_v<is_nothrow_constructible<_Elements,
> > > _UTypes>...>;
> > > else
> > > return false;
> > > }
> > > @@ -783,7 +783,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > __convertible()
> > > {
> > > if constexpr (sizeof...(_UTypes) == sizeof...(_Elements))
> > > - return (is_convertible_v<_UTypes, _Elements> && ...);
> > > + return __and_v<is_convertible<_UTypes, _Elements>...>;
> > > else
> > > return false;
> > > }
> > > @@ -1526,7 +1526,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > __assignable()
> > > {
> > > if constexpr (sizeof...(_UTypes) == sizeof...(_Elements))
> > > - return (is_assignable_v<_Elements&, _UTypes> && ...);
> > > + return __and_v<is_assignable<_Elements&, _UTypes>...>;
> > > else
> > > return false;
> > > }
> > > @@ -1536,7 +1536,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > __nothrow_assignable()
> > > {
> > > if constexpr (sizeof...(_UTypes) == sizeof...(_Elements))
> > > - return (is_nothrow_assignable_v<_Elements&, _UTypes> && ...);
> > > + return __and_v<is_nothrow_assignable<_Elements&, _UTypes>...>;
> > > else
> > > return false;
> > > }
> > > @@ -1547,7 +1547,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > __const_assignable()
> > > {
> > > if constexpr (sizeof...(_UTypes) == sizeof...(_Elements))
> > > - return (is_assignable_v<const _Elements&, _UTypes> && ...);
> > > + return __and_v<is_assignable<const _Elements&, _UTypes>...>;
> > > else
> > > return false;
> > > }
> > >
> > > Happier with that?
> > >
> > > It passes all the tuple tests, I'm running the full suite now.
> > >
> > >
> >
> > LGTM
>
> Pushed to trunk - thanks for the reviews.
I'm seeing a redefinition error when compiling <tuple> with
-std=c++20 -U__cpp_conditional_explicit (which IIUC is intended
to work?):
/home/ppalka/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/tuple:1536:9:
error: ‘template<class ... _Elements> template<class ... _UTypes> static
consteval bool std::tuple< <template-parameter-1-1> >::__nothrow_assignable()’
cannot be overloaded with ‘template<class ... _Elements> template<class ...
_UElements> static constexpr bool std::tuple< <template-parameter-1-1>
>::__nothrow_assignable()’
1536 | __nothrow_assignable()
| ^~~~~~~~~~~~~~~~~~~~
/home/ppalka/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/tuple:1248:31:
note: previous declaration ‘template<class ... _Elements> template<class ...
_UElements> static constexpr bool std::tuple< <template-parameter-1-1>
>::__nothrow_assignable()’
1248 | static constexpr bool __nothrow_assignable()
| ^~~~~~~~~~~~~~~~~~~~
