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 ? > > > * 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. > > > >@@ -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 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 ? I will address other tests comments when I have an agreement on what to do with the above issue. > > > >+// 2019-04-10 Nina Dinka Ranns <dinka.ra...@gmail.com> > >+// > >+// Copyright (C) 2017 Free Software Foundation, Inc. > > Copyright date on new files should be 2019. > > >+// > >+// This file is part of the GNU ISO C++ Library. This library is free > >+// software; you can redistribute it and/or modify it under the > >+// terms of the GNU General Public License as published by the > >+// Free Software Foundation; either version 3, or (at your option) > >+// any later version. > >+// > >+// This library is distributed in the hope that it will be useful, > >+// but WITHOUT ANY WARRANTY; without even the implied warranty of > >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >+// GNU General Public License for more details. > >+// > >+// You should have received a copy of the GNU General Public License along > >+// with this library; see the file COPYING3. If not see > >+// <http://www.gnu.org/licenses/>. > >+ > >+#include <tuple> > >+#include <testsuite_tr1.h> > >+#include <utility> > >+ > >+/* DefaultConstructionTests */ > >+ > >+using namespace __gnu_test; > >+ > >+bool throwing_ctor_called = false; > >+ > >+ > >+template<typename T> > >+bool checkDefaultThrowConstruct() > >+{ > >+ throwing_ctor_called = false; > >+ bool deduced_nothrow = std::is_nothrow_constructible<T>::value; > >+ T t{}; > >+ return throwing_ctor_called != deduced_nothrow; > >+} > >+ > >+ > >+typedef std::tuple<int> IT; > >+typedef std::tuple<const int> CIT; > >+typedef std::tuple<int&&> RVIT; > >+typedef std::tuple<int, int> IIT; > >+typedef std::pair<int, int> IIP; > >+typedef std::tuple<int, int, int> IIIT; > >+ > >+namespace DefaultConstructionTests > >+{ > >+ struct NoexceptDC > >+ { > >+ NoexceptDC() noexcept(true){} > >+ }; > >+ > >+ struct ExceptDC > >+ { > >+ ExceptDC() noexcept(false) > >+ { throwing_ctor_called = true; } > >+ }; > >+ > >+ struct ExplicitNoexceptDC > >+ { > >+ explicit ExplicitNoexceptDC() noexcept(true) > >+ {} > >+ }; > >+ > >+ struct ExplicitExceptDC > >+ { > >+ explicit ExplicitExceptDC() noexcept(false) > >+ { throwing_ctor_called = true; } > >+ }; > >+ > >+ typedef std::tuple<NoexceptDC> NDT; > >+ typedef std::tuple<ExceptDC> EDT; > >+ typedef std::tuple<ExplicitNoexceptDC> X_NDT; > >+ typedef std::tuple<ExplicitExceptDC> X_EDT; > >+ > >+ typedef std::tuple<NoexceptDC,NoexceptDC> NNDT; > >+ typedef std::tuple<ExceptDC,ExceptDC> EEDT; > >+ typedef std::tuple<ExceptDC,NoexceptDC> ENDT; > >+ typedef std::tuple<ExplicitNoexceptDC,NoexceptDC> X_NNDT; > >+ typedef std::tuple<ExplicitExceptDC,ExceptDC> X_EEDT; > >+ typedef std::tuple<ExceptDC,ExplicitNoexceptDC> X_ENDT; > >+ > >+ typedef std::tuple<long, NoexceptDC, NoexceptDC> LNDNDT; > >+ typedef std::tuple<long, NoexceptDC, ExceptDC> LNDEDT; > >+ typedef std::tuple<long, ExplicitNoexceptDC, NoexceptDC> X_LNEDNDT; > >+ typedef std::tuple<long, ExplicitNoexceptDC, ExceptDC> X_LNEDEDT; > >+ typedef std::tuple<long, ExplicitExceptDC, ExceptDC> X_LEEDEDT; > >+ > >+ > >+ /* if it has E in the name, it contains a type that throws when default > >constructed */ > >+ static_assert(std::is_nothrow_constructible<IT>::value, ""); > >+ static_assert(std::is_nothrow_constructible<NDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<EDT>::value, ""); > >+ static_assert(std::is_nothrow_constructible<X_NDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<X_EDT>::value, ""); > >+ > >+ static_assert(std::is_nothrow_constructible<IIT>::value, ""); > >+ static_assert(std::is_nothrow_constructible<NNDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<EEDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<ENDT>::value, ""); > >+ static_assert(std::is_nothrow_constructible<X_NNDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<X_EEDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<X_ENDT>::value, ""); > >+ > >+ static_assert(std::is_nothrow_constructible<IIIT>::value, ""); > >+ static_assert(std::is_nothrow_constructible<LNDNDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<LNDEDT>::value, ""); > >+ static_assert(std::is_nothrow_constructible<X_LNEDNDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<X_LNEDEDT>::value, ""); > >+ static_assert(!std::is_nothrow_constructible<X_LEEDEDT>::value, ""); > >+ > >+ void Run() > >+ { > >+ VERIFY( checkDefaultThrowConstruct<IT>() ); > >+ VERIFY( checkDefaultThrowConstruct<NDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<EDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<X_NDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<X_EDT>() ); > >+ > >+ VERIFY( checkDefaultThrowConstruct<IIT>() ); > >+ VERIFY( checkDefaultThrowConstruct<NNDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<EEDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<ENDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<X_NNDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<X_EEDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<X_ENDT>() ); > >+ > >+ VERIFY( checkDefaultThrowConstruct<IIIT>() ); > >+ VERIFY( checkDefaultThrowConstruct<LNDNDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<LNDEDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<X_LNEDNDT>() ); > >+ VERIFY( checkDefaultThrowConstruct<X_LNEDEDT>() ); > >+ } > >+} > >+ > >+ > >+int main() > >+{ > >+ > >+ DefaultConstructionTests::Run(); > >+ > >+} > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > >+ > > These blank lines should go. > > >Index: libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs2.cc > >=================================================================== > >--- libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs2.cc > >(nonexistent) > >+++ libstdc++-v3/testsuite/20_util/tuple/cons/noexcept_specs2.cc > >(working copy) > >@@ -0,0 +1,129 @@ > >+// { dg-options { -std=gnu++2a } } > >+// { dg-do run { target c++2a } } > > This one has an empty main() function, so could be a { dg-do compile } > test instead (and then it doesn't need a main() function). Although if > you combine all the new test files into one file then it will have a > non-empty main(), and will need to be { dg-do run }, so this comment > is just FYI really. > >