On Tue, 23 Apr 2019 at 21:28, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> 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).
I'd argue the trait is fine as it is. The semantics of conversion are
different in C++14 and C++17, and there is a theoretical possibility
that a conversion may involve a temporary in C++11/14. Like you say,
it's c++2a only, so it's a pointless 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);
clever :) Using list initialization skips the temporary. I like it. :)
>
> 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.
Done.
>
>
> >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)
Fixed (I think, it's a fiddly check in my environment so I may have
missed something)
>
> In the new test files you define is_nothrow_constructible_v but then
> never use it. It could be removed (or could be used).
Removed
>
> 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.
Renamed.
New diff attached.
Best,
Nina
noexcept_tuple_v3.diff
Description: Binary data
