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