On 23/04/19 18:43 +0100, Nina Dinka Ranns wrote:
On Thu, 18 Apr 2019 at 21:35, Jonathan Wakely <jwak...@redhat.com> wrote:
On 16/04/19 17:59 +0100, Nina Dinka Ranns wrote:
>On Tue, 16 Apr 2019 at 15:18, Jonathan Wakely <jwak...@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.ra...@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.
