On Thu, 18 Apr 2019 at 21:35, Jonathan Wakely <[email protected]> wrote: > > On 16/04/19 17:59 +0100, Nina Dinka Ranns wrote: > >On Tue, 16 Apr 2019 at 15:18, Jonathan Wakely <[email protected]> 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 <[email protected]> > >> > > >> > 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
>
noexcept_tuple_v2.diff
Description: Binary data
