Adding noexcept-specification on tuple constructors (LWG 2899)
Jonathan Wakely
jwakely@redhat.com
Tue Apr 23 21:05:00 GMT 2019
On 23/04/19 18:43 +0100, Nina Dinka Ranns wrote:
>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
Thanks for the demo. Arguably it's a misfeature of the
is_nothrow_convertible trait, and it should give the same result in
C++14 and C++17 (but as it only exists in C++2a that's not a very good
argument to make ;-). This topic came up on the LWG reflector recently
and https://wg21.link/lwg3194 was opened (the issue doesn't capture
the resulting discussion).
To make the trait's behaviour consistent we could use a
braced-init-list:
template<typename _From1, typename _To1>
static bool_constant<noexcept(__test_aux<_To1>({std::declval<_From1>()}))>
__test(int);
Both before and after C++17 this tests only whether the conversion
throws, and not whether the (possibly elided) copy/move construction
can throw. Defining your test_trait that way might be preferable,
because it means you can test *just* the converting constructor's
exception specification in isolation, rather than the combination of
two exception specifications on two constructors.
The result of is_nothrow_convertible is meaningful for user code
performing such conversions (i.e. if they're performing the conversion
in C++14 then they care about the move ctor, but in C++17 they don't).
But the purpose of our tests is to verify that the exception specs
you've added do the right thing. And for that, it's probably better to
have a trait that tests one thing at a time.
Unless I'm mistaken, if your tests used that trait instead of
is_nothrow_convertible as defined in C++2a, you wouldn't need separate
tests for C++11/14 and C++17/2a, right? You could just take the
noexcept_specs_cpp17.cc test and use it for all dialects.
>The new diff containing changes addressing review comments is attached
>to this e-mail.
Thanks. All the changes in <tuple> look good, except for some
whitespace errors that git noticed:
/dev/shm/noexcept_tuple_v2.diff:79: trailing whitespace.
explicit constexpr tuple(const tuple<_UElements...>& __in)
/dev/shm/noexcept_tuple_v2.diff:108: space before tab in indent.
is_nothrow_constructible<_T2, _U2>>::value;
/dev/shm/noexcept_tuple_v2.diff:119: space before tab in indent.
is_nothrow_default_constructible<_T2>>::value)
/dev/shm/noexcept_tuple_v2.diff:128: space before tab in indent.
is_nothrow_default_constructible<_T2>>::value)
In the new test files you define is_nothrow_constructible_v but then
never use it. It could be removed (or could be used).
The test_trait helper types doesn't need to use reserved names, i.e.
they could use From and To and test, instead of _From and _To and
__test.
More information about the Gcc-patches
mailing list