On 20/09/17 17:52 +0100, Jonathan Wakely wrote:
On 20/09/17 16:36 +0100, Jonathan Wakely wrote:
On 04/09/17 16:48 +0100, Jonathan Wakely wrote:
On 30/07/17 15:01 +0200, Daniel Krügler wrote:
2017-07-28 22:40 GMT+02:00 Daniel Krügler <daniel.krueg...@gmail.com>:
2017-07-28 22:29 GMT+02:00 Daniel Krügler <daniel.krueg...@gmail.com>:
2017-07-28 22:25 GMT+02:00 Tim Song <t.canens....@gmail.com>:
On Fri, Jul 28, 2017 at 4:10 PM, Daniel Krügler
<daniel.krueg...@gmail.com> wrote:
+ // Performs an implicit conversion from _Tp to __sv_type.
+ template<typename _Tp>
+ static __sv_type _S_to_string_view(const _Tp& __svt)
+ {
+ return __svt;
+ }
I might have gone for
+ static __sv_type _S_to_string_view(__sv_type __svt) noexcept
+ {
+ return __svt;
+ }
With that, we can also use noexcept(_S_to_string_view(__t)) to make up
for the absence of is_nothrow_convertible (basically the same thing I
did in LWG 2993's PR).
Agreed, that makes very much sense. I will adjust the P/R, but before
I resubmit I would like to get feedback whether the other two compare
functions also should become conditionally noexcept.
Locally I have now performed the sole change of the _S_to_string_view
declaration getting rid of the template, but would also like to gather
feedback from the maintainers whether I should also change the form of
the conditional noexcept to use the expression
noexcept(_S_to_string_view(__t))
instead of the current
is_same<_Tp, __sv_type>::value
as suggested by Tim Song.
I'm asking also, because I have a paper proposing to standardize
is_nothrow_convertible submitted for the upcoming C++ mailing - This
would be one of the first applications in the library ;-)
A slightly revised patch update: It replaces the _S_to_string_view
template by a simpler _S_to_string_view function as of Tim Song's
suggestion, but still uses the simplified noexcept specification
deferring it to a future application case for is_nothrow_convertible.
Furthermore now all three compare function templates are now
(conditionally) noexcept by an (off-list) suggestion from Jonathan
Wakely.
I've committed this, after some whitespace fixes and testing.
Thanks!
This change causes two regressions in C++17 mode, see
https://gcc.gnu.org/ml/gcc-testresults/2017-09/msg01674.html
FAIL: 21_strings/basic_string/cons/char/moveable2.cc execution test
FAIL: 21_strings/basic_string/cons/wchar_t/moveable2.cc execution test
Here's a reduced version of that test, which passes in C++14 and fails
in C++17:
#include <string>
#include <assert.h>
class tstring : public std::string
{
public:
tstring() : std::string() {}
tstring(tstring&& s) : std::string(std::move(s)) {}
};
int main()
{
tstring b;
b.push_back('1');
tstring c(std::move(b));
assert( c.size() == 1 && c[0] == '1' );
assert( b.size() == 0 );
}
The second assertion fails, because this mem-initializer:
tstring(tstring&& s) : std::string(std::move(s)) {}
now prefers to use the new constructor:
basic_string(const _Tp& __t, const _Alloc& __a = _Alloc())
because tstring is convertible to string_view.
This turns a non-allocating move into an allocating copy.
This patch fixes the failure above, I'm testing it now.
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -115,6 +115,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
template<typename _Tp, typename _Res>
using _If_sv = enable_if_t<
__and_<is_convertible<const _Tp&, __sv_type>,
+ __not_<is_convertible<const _Tp*, const basic_string*>>,
__not_<is_convertible<const _Tp&, const _CharT*>>>::value,
_Res>;
I'm committing this to fix the regression. If the final LWG 2946
resolution does something different we can change it again.
Tested powerpc64le-linux, committed to trunk.
I'll start another test run with all variations ...
commit 45143c77f09ff9ebe27761bca81ccd4f9f928d9b
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Wed Sep 20 17:56:53 2017 +0100
PR libstdc++/79162 Fix std::string regression due to LWG 2946
PR libstdc++/79162
* include/bits/basic_string.h (basic_string::_If_sv): Remove from the
overload set when the argument is derived from basic_string.
* testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc: New
test.
* testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc:
New test.
diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 0ef139b2c2b..8a382d5640c 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -115,6 +115,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
template<typename _Tp, typename _Res>
using _If_sv = enable_if_t<
__and_<is_convertible<const _Tp&, __sv_type>,
+ __not_<is_convertible<const _Tp*, const basic_string*>>,
__not_<is_convertible<const _Tp&, const _CharT*>>>::value,
_Res>;
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc
new file mode 100644
index 00000000000..88be60d05ab
--- /dev/null
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc
@@ -0,0 +1,53 @@
+// { dg-options "-std=gnu++17" }
+// { dg-do run { target c++17 } }
+
+// Copyright (C) 2011-2017 Free Software Foundation, Inc.
+//
+// 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/>.
+
+// NOTE: This makes use of the fact that we know how moveable
+// is implemented on string (via swap). If the implementation changed
+// this test may begin to fail.
+
+#include <string>
+#include <utility>
+#include <testsuite_hooks.h>
+
+class tstring : public std::basic_string<char>
+{
+public:
+ tstring() : std::basic_string<char>() {}
+ tstring(tstring&& s) : std::basic_string<char>(std::move(s)) {}
+ tstring& operator=(tstring&& s) = default;
+};
+
+void test01()
+{
+ tstring a, b;
+ a.push_back('1');
+ b = std::move(a);
+ VERIFY( b.size() == 1 && b[0] == '1' && a.size() == 0 );
+
+ tstring c(std::move(b));
+ VERIFY( c.size() == 1 && c[0] == '1' );
+ VERIFY( b.size() == 0 );
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc
new file mode 100644
index 00000000000..63684de68a6
--- /dev/null
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc
@@ -0,0 +1,53 @@
+// { dg-options "-std=gnu++17" }
+// { dg-do run { target c++17 } }
+
+// Copyright (C) 2011-2017 Free Software Foundation, Inc.
+//
+// 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/>.
+
+// NOTE: This makes use of the fact that we know how moveable
+// is implemented on string (via swap). If the implementation changed
+// this test may begin to fail.
+
+#include <string>
+#include <utility>
+#include <testsuite_hooks.h>
+
+class tstring : public std::basic_string<wchar_t>
+{
+public:
+ tstring() : std::basic_string<wchar_t>() {}
+ tstring(tstring&& s) : std::basic_string<wchar_t>(std::move(s)) {}
+ tstring& operator=(tstring&& s) = default;
+};
+
+void test01()
+{
+ tstring a, b;
+ a.push_back(L'1');
+ b = std::move(a);
+ VERIFY( b.size() == 1 && b[0] == L'1' && a.size() == 0 );
+
+ tstring c(std::move(b));
+ VERIFY( c.size() == 1 && c[0] == L'1' );
+ VERIFY( b.size() == 0 );
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
