Re: Adding noexcept-specification on tuple constructors (LWG 2899)

[Nina Dinka Ranns <dinka dot ranns at gmail dot com>]

On Thu, 18 Apr 2019 at 21:35, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 16/04/19 17:59 +0100, Nina Dinka Ranns wrote:
> >On Tue, 16 Apr 2019 at 15:18, Jonathan Wakely <jwakely@redhat.com> wrote:
> >>
> >> On 16/04/19 14:08 +0100, Nina Dinka Ranns wrote:
> >> >Tested on Linux-PPC64
> >> >Adding noexcept-specification on tuple constructors (LWG 2899)
> >>
> >> Thanks, Nina!
> >>
> >> This looks great, although as I think Ville has explained we won't
> >> commit it until the next stage 1, after the GCC 9 release.
> >ack
> >
> >>
> >> The changes look good, I just have some mostly-stylistic comments,
> >> which are inline below ...
> >>
> >>
> >> >2019-04-13 Nina Dinka Ranns <dinka.ranns@gmail.com>
> >> >
> >> >        Adding noexcept-specification on tuple constructors (LWG 2899)
> >> >        * libstdc++-v3/include/std/tuple:
> >> >        (tuple()): Add noexcept-specification.
> >> >        (tuple(const _Elements&...)): Likewise
> >> >        (tuple(_UElements&&...)): Likewise
> >> >        (tuple(const tuple<_UElements...>&)): Likewise
> >> >        (tuple(tuple<_UElements...>&&)): Likewise
> >> >        (tuple(const _T1&, const _T2&)): Likewise
> >> >        (tuple(_U1&&, _U2&&)): Likewise
> >> >        (tuple(const tuple<_U1, _U2>&): Likewise
> >> >        (tuple(tuple<_U1, _U2>&&): Likewise
> >> >        (tuple(const pair<_U1, _U2>&): Likewise
> >> >        (tuple(pair<_U1, _U2>&&): Likewise
> >> >
> >> >
> >>
> >> There should be no blank lines in the changelog entry here. A single
> >> change should be recorded as a single block in the changelog, with no
> >> blank lines within it.
> >ack. Do you need me to do anything about this or is it for future
> >reference only ?
>
> For future reference. Whoever commits the patch can correct the
> changelog.
>
> >>
> >> >        * libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs.cc: New
> >> >        * libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs2.cc: New
> >> >        * libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs3.cc: New
> >> >        * libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs4.cc: New
> >> >        * libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs5.cc: New
> >> >        * libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs6.cc: New
> >>
> >> This is a lot of new test files for a small-ish QoI feature. Could
> >> they be combined into one file?  Generally we do want one test file
> >> per feature, but I think all of these are arguably testing one feature
> >> (just on different constructors). The downside of lots of smaller
> >> files is that we have to compile+assemble+link+run each one, which
> >> adds several fork()s to launch a new process for each step. On some
> >> platforms that can be quite slow.
> >I can do that, but there may be an issue. See below.
> >
> >>
> >>
> >> >@@ -624,6 +634,7 @@
> >> >                   && (sizeof...(_Elements) >= 1),
> >> >         bool>::type=true>
> >> >         constexpr tuple(_UElements&&... __elements)
> >> >+        noexcept(__and_<is_nothrow_constructible<_Elements,_UElements&&>...>::value)
> >>
> >> Can this be __nothrow_constructible<_UElements>() ?
> >It should have been that in the first place. Apologies. Fixed.
> >
> >
> >>
> >> >         : _Inherited(std::forward<_UElements>(__elements)...) { }
> >> >
> >> >       template<typename... _UElements, typename
> >> >@@ -635,6 +646,7 @@
> >> >                   && (sizeof...(_Elements) >= 1),
> >> >         bool>::type=false>
> >> >         explicit constexpr tuple(_UElements&&... __elements)
> >> >+        noexcept(__nothrow_constructible<_UElements&&...>())
> >>
> >> The && here is redundant, though harmless.
> >>
> >> is_constructible<T,U&&> is exactly equivalent to is_constructible<T,U>
> >> because U means construction from an rvalue of type U and so does U&&.
> >>
> >> It's fine to leave the && there though.
> >I'm happy to go either way. The only reason I used && form is because
> >it mimics the wording in the LWG resolution.
>
> I suspect if STL had reviewed the wording in the resolution he'd have
> asked for the && to be removed :-)
:) ack. Removed.


>
>
> >> >@@ -966,6 +995,7 @@
> >> >                 && !is_same<__remove_cvref_t<_U1>, allocator_arg_t>::value,
> >> >       bool>::type = true>
> >> >         constexpr tuple(_U1&& __a1, _U2&& __a2)
> >> >+        noexcept(__nothrow_constructible<_U1&&,_U2&&>())
> >>
> >> There should be a space after the comma here, and all the later
> >> additions in the file.
> >ack. Fixed
> >
> >>
> >>
> >> >Index: libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs.cc
> >> >===================================================================
> >> >--- libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs.cc        (nonexistent)
> >> >+++ libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs.cc        (working copy)
> >> >@@ -0,0 +1,191 @@
> >> >+// { dg-options { -std=gnu++2a } }
> >> >+// { dg-do run { target c++2a } }
> >>
> >> This new file doesn't use std::is_nothrow_convertible so could just
> >> use: { dg-do run { target c++11 } } and no dg-options.
> >>
> >> For the other new tests that do use is_nothrow_convertible, I'm
> >> already planning to add std::is_nothrow_convertible for our internal
> >> use in C++11, so they could use that.
> >>
> >> Alternatively, the test files themselves could define:
> >>
> >> template<typename From, typename To>
> >>   struct is_nothrow_convertible
> >>   : std::integral_constant<bool,
> >>       is_convertible<From, To> && is_nothrow_constructible<Fo, From>>
> >>   { };
> >>
> >> and then use that. That way we can test the exception specs are
> >> correct in C++11 mode, the default C++14 mode, and C++17 mode.
> >> Otherwise we're adding code that affects all those modes but only
> >> testing it works correctly for the experimental C++2a mode.
> >
> >There is a reason why the tests are only C++2a mode. The semantics of
> >copy construction changed between C++14 and C++17, the effect of which
> >is that the is_nothrow_convertible (or its equivalent work around) in
> >certain cases doesn't evaluate the same before and after C++17. After
> >discussing this with Ville, we decided to only test C++2a because
> >that's the target of the library issue and because C++2a provided
>
> But this isn't actually related to the library issue. The proposed
> resolution that you're implementing doesn't fix the issue! The
> discussion in the issue says "The description doesn't match the
> resolution" and that's correct. That's why I've provided a new
> resolution to the issue. What you're doing is a Good Thing anyway,
> even if it doesn't solve LWG 2899. But since it isn't a fix for 2899,
> which version of C++ the issue targets is irrelevant. You're applying
> a change that affects std::tuple unconditionally, from C++11 onwards.
> You're changing the code for C++11, so it should be tested for C++11.
>
> It would be appropriate to only test C++2a if you were adding
> exception specifications that only applied for C++2a, like so:
>
>         constexpr tuple(_UElements&&... __elements)
> #if __cplusplus > 201703L
>         noexcept(__and_<is_nothrow_constructible<_Elements,_UElements&&>...>::value)
> #endif
>
> But that's not what your patch does (and not what we want it to do).
>
> So I really think we need *some* tests that can run in C++11 mode.
> They wouldn't have to be as extensive and thorough as the ones you've
> provided, but if we make changes that affect C++11/14/17 then we
> should test those changes.
Ok, I now have a set of tests for c++11/14 and a set for C++17/20.

>
> >std::is_nothrow_convertible so I could do away with my copy conversion
> >helper functions.
> >Extending the tests to earlier standards would mean having two sets of
> >test expectations (one for pre C++17 copy conversion semantics, and
> >one for C++17 onwards). Would you like me to do that ?
>
> Can you show an example of the different semantics? What behaves
> differently in C++14?
>
> If the library provided a std::__is_nothrow_convertible trait for
> C++11 (with the same implementation as C++20's
> std::is_nothrow_convertible) and your tests used that, which test
> expectations would be affected?

Copy initialization pre C++17 involved creating a temporary of
destination type and then copying/moving the temporary to the result.
This means that a hypothetical std::__is_nothrow_convertible
implementation pre C++17 involves an additional call to a copy/move
constructor and that in some cases changes the nothrow outcome.

Here is an example of such a case :
https://wandbox.org/permlink/PppTzLXrZH2Dk4eo

It will give different result for C++11/14 and C++17/20

The new diff containing changes addressing review comments is attached
to this e-mail.

Let me know what you think,
Nina

>

Attachment: noexcept_tuple_v2.diff
Description: Binary data