On Thu, 18 Apr 2019 at 21:35, Jonathan Wakely wrote: > > On 16/04/19 17:59 +0100, Nina Dinka Ranns wrote: > >On Tue, 16 Apr 2019 at 15:18, Jonathan Wakely 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 > >> > > >> > 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_...>::value) > >> > >> Can this be __nothrow_constructible<_UElements>() ? > >It should have been that in the first place. Apologies. Fixed. > > > > > >> > >> > : _Inherited(std::forward<_UElements>(__elements)...) { } > >> > > >> > template >> >@@ -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 is exactly equivalent to is_constructible > >> 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 > >> struct is_nothrow_convertible > >> : std::integral_constant >> is_convertible && is_nothrow_constructible> > >> { }; > >> > >> 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_...>::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 >