Adding noexcept-specification on tuple constructors (LWG 2899)

[Jonathan Wakely]

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 :-)


>> >@@ -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.

>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?