| Message ID | 53722923-e31e-4311-27cb-c66d60b52b19@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Complete __gnu_debug::basic_string | expand |
On 16/03/21 21:55 +0100, François Dumont via Libstdc++ wrote: >Following: > >https://gcc.gnu.org/pipermail/libstdc++/2021-March/052158.html > >Here is the patch to complete __gnu_debug::basic_string support. >Contrarily to what I thought code in std::basic_string to generate a >basic_string_view works just fine for __gnu_debug::basic_string. > > libstdc++: [_GLIBCXX_DEBUG] Add __gnu_debug >u8string/u16string/u32string > > Complete __gnu_debug::basic_string support so that it can be used as a > transparent replacement of std::basic_string. > > libstdc++-v3/ChangeLog: > > * include/debug/string > (basic_string(const _CharT*, const _Allocator&)): Remove >assign call. > (basic_string<>::insert(const_iterator, _InputIte, >_InputIte)): Try to > remove iterator debug layer even if !_GLIBCXX_USE_CXX11_ABI. > [_GLIBCXX_USE_CHAR8_T] (__gnu_debug::u8string): New. > (__gnu_debug::u16string, __gnu_debug::u32string): New. >[!_GLIBCXX_COMPATIBILITY_CXX0X](std::hash<__gnu_debug::string>): New. >[!_GLIBCXX_COMPATIBILITY_CXX0X][_GLIBCXX_USE_WCHAR_T](std::hash<__gnu_debug::wstring>): >New. >[_GLIBCXX_USE_CHAR8_T](std::hash<__gnu_debug::u8string>): New. > (std::hash<__gnu_debug::u16string>): New. > (std::hash<__gnu_debug::u32string>): New. > * testsuite/21_strings/basic_string/hash/hash_char8_t.cc: >Adapt for > __gnu_debug basic_string. > >Tested under Linux x86_64. > >Ok to commit ? > >François > >diff --git a/libstdc++-v3/include/debug/string b/libstdc++-v3/include/debug/string >index d6eb5280ade..dec23f6277b 100644 >--- a/libstdc++-v3/include/debug/string >+++ b/libstdc++-v3/include/debug/string >@@ -41,6 +41,14 @@ > __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func) \ > ._M_message(#_Cond)._M_error() > >+#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 >+# define _GLIBCXX_CPP11_AND_CXX11_ABI 1 >+# define _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY(Statement) Statement This takes an expression, not a statement. I think it would be better to use more descriptive names for these: # define _GLIBCXX_INSERT_RETURNS_ITERATOR 1 # define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) expr (And don't forget to change the #undef lines too). >+#if __cplusplus >= 201103L >+ >+namespace std _GLIBCXX_VISIBILITY(default) >+{ >+_GLIBCXX_BEGIN_NAMESPACE_VERSION >+ >+ // DR 1182. >+ >+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >+ /// std::hash specialization for string. >+ template<> >+ struct hash<__gnu_debug::string> >+ : public __hash_base<size_t, __gnu_debug::string> I think we could just define on partial specialization that works for all cases: template<typename _CharT> struct hash<__gnu_debug::basic_string<_CharT>> : public hash<std::basic_string<_CharT>> { }; If we're being pedantic, this isn't strictly-conforming because it will inherit the argument_type member from the base class, which will be the non-debug string rather than the debug one. I don't think I care about that. If we cared, we'd need to do something like: template<typename _CharT, typename = void> struct __debug_str_hash { }; template<typename _CharT> struct __debug_str_hash<_CharT, __void_t<typename hash<std::basic_string<_CharT>>::argument_type>> : hash<std::basic_string<_CharT>> { using argument_type = __gnu_debug::basic_string<_CharT>; size_t operator()(const argument_type& __s) const noexcept { return hash<std::basic_string<_CharT>>()(__s); } }; template<typename _CharT> struct hash<__gnu_debug::basic_string<_CharT>> : public __debug_str_hash<_CharT> { }; I don't think we should care. >+ { >+ size_t >+ operator()(const __gnu_debug::string& __s) const noexcept >+ { return std::hash<std::string>()(__s); } >+ }; >+ >+ template<> >+ struct __is_fast_hash<hash<__gnu_debug::string>> : std::false_type >+ { }; And: template<typename _CharT> struct __is_fast_hash<hash<__gnu_debug::basic_string<_CharT>>> : __is_fast_hash<hash<std::basic_string<_CharT>>> { }; >diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc b/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc >index c0e8975ce4a..7f8792bbd76 100644 >--- a/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc >+++ b/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc Why only make this change for the char8_t version? Why not test hash<__gnu_debug::string> as well? >@@ -18,10 +18,19 @@ > // { dg-options "-std=gnu++2a" } > // { dg-do run { target c++2a } } > >-#include <string> > #include <memory_resource> > #include <testsuite_hooks.h> > >+#ifdef _GLIBCXX_DEBUG >+# include <debug/string> >+ >+using namespace __gnu_debug; This means we no longer test the hash<std::u8string> specialization in debug mode. Please leave this test unchanged and add a separate file (e.g. 21_strings/basic_string/hash/debug.cc) which tests all the hash<__gnu_debug::*string> specializations. It's stage 4, but this only affects a type that is not used anywhere by default, not even when Debug Mode is active, so I think this can still potentially go in for GCC 11.
Following your feedback here is the simplified version. I grouped it with the patch I submitted before. On 19/03/21 8:41 pm, Jonathan Wakely wrote: > On 16/03/21 21:55 +0100, François Dumont via Libstdc++ wrote: >> Following: >> >> https://gcc.gnu.org/pipermail/libstdc++/2021-March/052158.html >> >> Here is the patch to complete __gnu_debug::basic_string support. >> Contrarily to what I thought code in std::basic_string to generate a >> basic_string_view works just fine for __gnu_debug::basic_string. >> >> libstdc++: [_GLIBCXX_DEBUG] Add __gnu_debug >> u8string/u16string/u32string >> >> Complete __gnu_debug::basic_string support so that it can be used >> as a >> transparent replacement of std::basic_string. >> >> libstdc++-v3/ChangeLog: >> >> * include/debug/string >> (basic_string(const _CharT*, const _Allocator&)): Remove >> assign call. >> (basic_string<>::insert(const_iterator, _InputIte, >> _InputIte)): Try to >> remove iterator debug layer even if !_GLIBCXX_USE_CXX11_ABI. >> [_GLIBCXX_USE_CHAR8_T] (__gnu_debug::u8string): New. >> (__gnu_debug::u16string, __gnu_debug::u32string): New. >> [!_GLIBCXX_COMPATIBILITY_CXX0X](std::hash<__gnu_debug::string>): New. >> [!_GLIBCXX_COMPATIBILITY_CXX0X][_GLIBCXX_USE_WCHAR_T](std::hash<__gnu_debug::wstring>): >> New. >> [_GLIBCXX_USE_CHAR8_T](std::hash<__gnu_debug::u8string>): New. >> (std::hash<__gnu_debug::u16string>): New. >> (std::hash<__gnu_debug::u32string>): New. >> * testsuite/21_strings/basic_string/hash/hash_char8_t.cc: >> Adapt for >> __gnu_debug basic_string. >> >> Tested under Linux x86_64. >> >> Ok to commit ? >> >> François >> > >> diff --git a/libstdc++-v3/include/debug/string >> b/libstdc++-v3/include/debug/string >> index d6eb5280ade..dec23f6277b 100644 >> --- a/libstdc++-v3/include/debug/string >> +++ b/libstdc++-v3/include/debug/string >> @@ -41,6 +41,14 @@ >> __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func) \ >> ._M_message(#_Cond)._M_error() >> >> +#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 >> +# define _GLIBCXX_CPP11_AND_CXX11_ABI 1 >> +# define _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY(Statement) Statement > > This takes an expression, not a statement. I've been inspired by _GLIBCXX_DEBUG_ONLY > > I think it would be better to use more descriptive names for these: > > # define _GLIBCXX_INSERT_RETURNS_ITERATOR 1 > # define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) expr > > (And don't forget to change the #undef lines too). > >> +#if __cplusplus >= 201103L >> + >> +namespace std _GLIBCXX_VISIBILITY(default) >> +{ >> +_GLIBCXX_BEGIN_NAMESPACE_VERSION >> + >> + // DR 1182. >> + >> +#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >> + /// std::hash specialization for string. >> + template<> >> + struct hash<__gnu_debug::string> >> + : public __hash_base<size_t, __gnu_debug::string> > > I think we could just define on partial specialization that works for > all cases: Yes, sounds better. But I relied on std::__hash_base which gives directly the correct definition. > Why only make this change for the char8_t version? Why not test > hash<__gnu_debug::string> as well? This file also test std::string and so also __gnu_debug::string. libstdc++: Fix and complete __gnu_debug::basic_string implementation Fix and complete __gnu_debug::basic_string so that it can be used as a transparent replacement of std::basic_string. libstdc++-v3/ChangeLog: * include/debug/string (basic_string(const _CharT*, const _Allocator&)): Remove assign call. (basic_string<>::insert(const_iterator, _InputIte, _InputIte)): Try to remove iterator debug layer even if !_GLIBCXX_USE_CXX11_ABI. [_GLIBCXX_USE_CHAR8_T] (__gnu_debug::u8string): New. (__gnu_debug::u16string, __gnu_debug::u32string): New. (std::hash<__gnu_debug::basic_string<>>): New partial specialization. (std::__is_fast_hash<__gnu_debug::basic_string<>>): Likewise. (basic_string(const basic_string&, const _Alloc&)): Define even if !_GLIBCXX_USE_CXX11_ABI. (basic_string(basic_string&&, const _Alloc&)): Likewise and add noexcept qualification. (basic_string<>::erase): Adapt to take __const_iterator. * testsuite/21_strings/basic_string/hash/debug.cc: New test. * testsuite/21_strings/basic_string/hash/debug_char8_t.cc: New test. * testsuite/21_strings/basic_string/requirements/citerators.cc: Adapt to test __gnu_debug::string when _GLIBCXX_DEBUG is defined. * testsuite/21_strings/basic_string/requirements/dr438/constructor.cc: Likewise. * testsuite/21_strings/basic_string/requirements/exception/basic.cc: Likewise. * testsuite/21_strings/basic_string/requirements/exception/generation_prohibited.cc: Likewise. * testsuite/21_strings/basic_string/requirements/exception/propagation_consistent.cc: Likewise. * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char/1.cc: Likewise. * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char16_t/1.cc: Likewise. * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char32_t/1.cc: Likewise. * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char8_t/1.cc: Likewise. * testsuite/21_strings/basic_string/requirements/explicit_instantiation/wchar_t/1.cc: Likewise. * testsuite/21_strings/basic_string/requirements/typedefs.cc: Likewise. * testsuite/util/exception/safety.h (erase_base<__gnu_debug::basic_string<>>): New partial specialization. (insert_base<__gnu_debug::basic_string<>>): Likewise. * testsuite/util/testsuite_container_traits.h (traits<__gnu_debug::basic_string<>>): Likewise. Tested under Linux x86_64. Ok to commit ? François
On 20/03/21 22:32 +0100, François Dumont wrote: >Following your feedback here is the simplified version. I grouped it >with the patch I submitted before. > > >On 19/03/21 8:41 pm, Jonathan Wakely wrote: >>On 16/03/21 21:55 +0100, François Dumont via Libstdc++ wrote: >>>Following: >>> >>>https://gcc.gnu.org/pipermail/libstdc++/2021-March/052158.html >>> >>>Here is the patch to complete __gnu_debug::basic_string support. >>>Contrarily to what I thought code in std::basic_string to generate >>>a basic_string_view works just fine for __gnu_debug::basic_string. >>> >>> libstdc++: [_GLIBCXX_DEBUG] Add __gnu_debug >>>u8string/u16string/u32string >>> >>> Complete __gnu_debug::basic_string support so that it can be >>>used as a >>> transparent replacement of std::basic_string. >>> >>> libstdc++-v3/ChangeLog: >>> >>> * include/debug/string >>> (basic_string(const _CharT*, const _Allocator&)): >>>Remove assign call. >>> (basic_string<>::insert(const_iterator, _InputIte, >>>_InputIte)): Try to >>> remove iterator debug layer even if !_GLIBCXX_USE_CXX11_ABI. >>> [_GLIBCXX_USE_CHAR8_T] (__gnu_debug::u8string): New. >>> (__gnu_debug::u16string, __gnu_debug::u32string): New. >>>[!_GLIBCXX_COMPATIBILITY_CXX0X](std::hash<__gnu_debug::string>): New. >>>[!_GLIBCXX_COMPATIBILITY_CXX0X][_GLIBCXX_USE_WCHAR_T](std::hash<__gnu_debug::wstring>): >>>New. >>>[_GLIBCXX_USE_CHAR8_T](std::hash<__gnu_debug::u8string>): New. >>> (std::hash<__gnu_debug::u16string>): New. >>> (std::hash<__gnu_debug::u32string>): New. >>> * >>>testsuite/21_strings/basic_string/hash/hash_char8_t.cc: Adapt for >>> __gnu_debug basic_string. >>> >>>Tested under Linux x86_64. >>> >>>Ok to commit ? >>> >>>François >>> >> >>>diff --git a/libstdc++-v3/include/debug/string >>>b/libstdc++-v3/include/debug/string >>>index d6eb5280ade..dec23f6277b 100644 >>>--- a/libstdc++-v3/include/debug/string >>>+++ b/libstdc++-v3/include/debug/string >>>@@ -41,6 +41,14 @@ >>> __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func) \ >>> ._M_message(#_Cond)._M_error() >>> >>>+#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 >>>+# define _GLIBCXX_CPP11_AND_CXX11_ABI 1 >>>+# define _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY(Statement) Statement >> >>This takes an expression, not a statement. > >I've been inspired by _GLIBCXX_DEBUG_ONLY Which is generally used with complete statements, not just expressions. See _GLIBCXX20_ONLY which is used for expressions, and uses __expr for the argument name. >> >>I think it would be better to use more descriptive names for these: >> >># define _GLIBCXX_INSERT_RETURNS_ITERATOR 1 >># define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) expr >> >>(And don't forget to change the #undef lines too). >> >>>+#if __cplusplus >= 201103L >>>+ >>>+namespace std _GLIBCXX_VISIBILITY(default) >>>+{ >>>+_GLIBCXX_BEGIN_NAMESPACE_VERSION >>>+ >>>+ // DR 1182. >>>+ >>>+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >>>+ /// std::hash specialization for string. >>>+ template<> >>>+ struct hash<__gnu_debug::string> >>>+ : public __hash_base<size_t, __gnu_debug::string> >> >>I think we could just define on partial specialization that works for >>all cases: > >Yes, sounds better. But I relied on std::__hash_base which gives >directly the correct definition. But that is wrong. The requirements in https://wg21.link/unord.hash say that std::hash<T> must be disabled for unsupported types. With std::basic_string the specialization std::hash<basic_string<int>> is disabled, because there is no specialization for that type, so it uses the primary template of std::hash, which is empty: /// Primary class template hash, usable for enum types only. // Use with non-enum types still SFINAES. template<typename _Tp> struct hash : __hash_enum<_Tp> { }; With your patch, std::hash<__gnu_debug::basic_string<int>> will not be empty. It will provide argument_type and result_type and operator() but calling operator() will fail to compile. My suggestion doesn't have that problem. With my suggestion, hash<_gnu_debug::basic_string<C>> is enabled if (and only if) hash<std::basic_string<C>> is enabled. >>Why only make this change for the char8_t version? Why not test >>hash<__gnu_debug::string> as well? > >This file also test std::string and so also __gnu_debug::string. Ah yes. > libstdc++: Fix and complete __gnu_debug::basic_string implementation > > Fix and complete __gnu_debug::basic_string so that it can be used >as a transparent > replacement of std::basic_string. > > libstdc++-v3/ChangeLog: > > * include/debug/string > (basic_string(const _CharT*, const _Allocator&)): Remove >assign call. > (basic_string<>::insert(const_iterator, _InputIte, >_InputIte)): Try to > remove iterator debug layer even if !_GLIBCXX_USE_CXX11_ABI. > [_GLIBCXX_USE_CHAR8_T] (__gnu_debug::u8string): New. > (__gnu_debug::u16string, __gnu_debug::u32string): New. > (std::hash<__gnu_debug::basic_string<>>): New partial >specialization. >(std::__is_fast_hash<__gnu_debug::basic_string<>>): Likewise. > (basic_string(const basic_string&, const _Alloc&)): Define >even if !_GLIBCXX_USE_CXX11_ABI. > (basic_string(basic_string&&, const _Alloc&)): Likewise >and add noexcept qualification. > (basic_string<>::erase): Adapt to take __const_iterator. > * testsuite/21_strings/basic_string/hash/debug.cc: New test. > * testsuite/21_strings/basic_string/hash/debug_char8_t.cc: >New test. > * >testsuite/21_strings/basic_string/requirements/citerators.cc: Adapt to >test __gnu_debug::string > when _GLIBCXX_DEBUG is defined. > * >testsuite/21_strings/basic_string/requirements/dr438/constructor.cc: >Likewise. > * >testsuite/21_strings/basic_string/requirements/exception/basic.cc: >Likewise. > * testsuite/21_strings/basic_string/requirements/exception/generation_prohibited.cc: >Likewise. > * testsuite/21_strings/basic_string/requirements/exception/propagation_consistent.cc: >Likewise. > * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char/1.cc: >Likewise. > * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char16_t/1.cc: >Likewise. > * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char32_t/1.cc: >Likewise. > * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char8_t/1.cc: >Likewise. > * testsuite/21_strings/basic_string/requirements/explicit_instantiation/wchar_t/1.cc: >Likewise. > * >testsuite/21_strings/basic_string/requirements/typedefs.cc: Likewise. > * testsuite/util/exception/safety.h >(erase_base<__gnu_debug::basic_string<>>): New partial > specialization. > (insert_base<__gnu_debug::basic_string<>>): Likewise. > * testsuite/util/testsuite_container_traits.h >(traits<__gnu_debug::basic_string<>>): Likewise. > > >Tested under Linux x86_64. > >Ok to commit ? > >François > >diff --git a/libstdc++-v3/include/debug/string b/libstdc++-v3/include/debug/string >index 172179530aa..f665c759557 100644 >--- a/libstdc++-v3/include/debug/string >+++ b/libstdc++-v3/include/debug/string >@@ -41,6 +41,14 @@ > __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func) \ > ._M_message(#_Cond)._M_error() > >+#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 >+# define _GLIBCXX_INSERT_RETURNS_ITERATOR 1 >+# define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) expr >+#else >+# define _GLIBCXX_INSERT_RETURNS_ITERATOR 0 >+# define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) >+#endif >+ > namespace __gnu_debug > { > /** Checks that __s is non-NULL or __n == 0, and then returns __s. */ >@@ -123,21 +131,21 @@ namespace __gnu_debug > > using _Base::npos; > >- basic_string() >- _GLIBCXX_NOEXCEPT_IF(std::is_nothrow_default_constructible<_Base>::value) >- : _Base() { } >- > // 21.3.1 construct/copy/destroy: >+ > explicit > basic_string(const _Allocator& __a) _GLIBCXX_NOEXCEPT > : _Base(__a) { } > > #if __cplusplus < 201103L >+ basic_string() : _Base() { } >+ > basic_string(const basic_string& __str) > : _Base(__str) { } > > ~basic_string() { } > #else >+ basic_string() = default; > basic_string(const basic_string&) = default; > basic_string(basic_string&&) = default; > >@@ -146,13 +154,15 @@ namespace __gnu_debug > : _Base(__l, __a) > { } > >-#if _GLIBCXX_USE_CXX11_ABI > basic_string(const basic_string& __s, const _Allocator& __a) > : _Base(__s, __a) { } > > basic_string(basic_string&& __s, const _Allocator& __a) >- : _Base(std::move(__s), __a) { } >-#endif >+ noexcept( noexcept( >+ _Base(std::declval<_Base>()), std::declval<const _Allocator&>()) ) There is a closing ')' in the wrong place here. This checks whether _Base is nothrow_move_constructible and whether std::declval is nothrow. You could just use noexcept(is_nothrow_constructible<_Base, _Base, const _Allocator&>::value) or: noexcept(noexcept(_Base(static_cast<_Base&&>(__s), __a))) It's a bit confusing that we have a noexcept specifier that only depends on _Base when the _Safe base class can also throw: >+ : _Safe(std::move(__s._M_safe()), __a), >+ _Base(std::move(__s._M_base()), __a) >+ { } In fact, it is conditionally noexcept with the same condition as the _Base type, so checking if the _Base construction is non-throwing is correct. But confusing. >+ /// std::hash specialization for __gnu_debug::basic_string. >+ template<typename _CharT> >+ struct hash<__gnu_debug::basic_string<_CharT>> >+ : public __hash_base<size_t, __gnu_debug::basic_string<_CharT>> >+ { >+ size_t >+ operator()(const __gnu_debug::basic_string<_CharT>& __s) const noexcept >+ { return std::hash<std::basic_string<_CharT>>()(__s); } >+ }; >+ >+ template<typename _CharT> >+ struct __is_fast_hash<hash<__gnu_debug::basic_string<_CharT>>> >+ : std::false_type This says it's always a slow hash, rather than deferring to __is_fast_hash<hash<std::basic_string<C>>>. That means __is_fast_hash is false for __gnu_debug::basic_string<int> but true for std::basic_string<int>. In practice it probably doesn't matter, but it's inconsistent. >+ { }; >+ >+_GLIBCXX_END_NAMESPACE_VERSION >+} >+#endif /* C++11 */ >+ >+#undef _GLIBCXX_INSERT_RETURNS_ITERATOR >+#undef _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY >+ > #endif >diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc b/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >new file mode 100644 >index 00000000000..84c989590b7 >--- /dev/null >+++ b/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >@@ -0,0 +1,53 @@ >+// Copyright (C) 2021 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/>. >+ >+// { dg-options "-std=gnu++17" } >+// { dg-do run { target c++17 } } -std=gnu++17 is the default now, so should not be given explicitly. You could combine this test and debug/hash_char8_t.cc by adding the char8_t parts guarded by _GLIBCXX_USE_CHAR8_T. When the test is run with a -std=gnu++20 it will test the char8_t parts (which is why we don't want the redundant -std=gnu++17, because it would prevent it from being run with -std=gnu++20). >diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc b/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >index 99bf5726dcc..69d4a8d0e51 100644 >--- a/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >+++ b/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >@@ -18,14 +18,21 @@ > // with this library; see the file COPYING3. If not see > // <http://www.gnu.org/licenses/>. > >-#include <string> > #include <testsuite_containers.h> > >+#ifdef _GLIBCXX_DEBUG >+# include <debug/string> >+using namespace __gnu_debug; >+#else >+# include <string> >+using namespace std; >+#endif This has the same problem I pointed out previously. With this change, we don't run this test for std::string in debug mode. When debug mode is active, we test a different type not std::string. That means if we introduce a bug to std::string that only affects debug mode, we might not notice, because we're stopped testing std::string in debug mode. If you want to test __gnu_debug::basic_string then those tests should be added as new tests, not by replacing existing tests that are already testing something different.
On 23/03/21 4:42 pm, Jonathan Wakely wrote: > On 20/03/21 22:32 +0100, François Dumont wrote: >> Following your feedback here is the simplified version. I grouped it >> with the patch I submitted before. >> >> >> On 19/03/21 8:41 pm, Jonathan Wakely wrote: >>> I think we could just define on partial specialization that works for >>> all cases: >> >> Yes, sounds better. But I relied on std::__hash_base which gives >> directly the correct definition. > > But that is wrong. > > The requirements in https://wg21.link/unord.hash say that std::hash<T> > must be disabled for unsupported types. With std::basic_string the > specialization std::hash<basic_string<int>> is disabled, because there > is no specialization for that type, so it uses the primary template of > std::hash, which is empty: > > /// Primary class template hash, usable for enum types only. > // Use with non-enum types still SFINAES. > template<typename _Tp> > struct hash : __hash_enum<_Tp> > { }; > > With your patch, std::hash<__gnu_debug::basic_string<int>> will not be > empty. It will provide argument_type and result_type and operator() > but calling operator() will fail to compile. > > My suggestion doesn't have that problem. With my suggestion, > hash<_gnu_debug::basic_string<C>> is enabled if (and only if) > hash<std::basic_string<C>> is enabled. Ok, I adopted your approach then. > >> libstdc++: Fix and complete __gnu_debug::basic_string implementation >> >> Fix and complete __gnu_debug::basic_string so that it can be used >> as a transparent >> replacement of std::basic_string. >> >> libstdc++-v3/ChangeLog: >> >> * include/debug/string >> (basic_string(const _CharT*, const _Allocator&)): Remove >> assign call. >> (basic_string<>::insert(const_iterator, _InputIte, >> _InputIte)): Try to >> remove iterator debug layer even if !_GLIBCXX_USE_CXX11_ABI. >> [_GLIBCXX_USE_CHAR8_T] (__gnu_debug::u8string): New. >> (__gnu_debug::u16string, __gnu_debug::u32string): New. >> (std::hash<__gnu_debug::basic_string<>>): New partial specialization. >> (std::__is_fast_hash<__gnu_debug::basic_string<>>): Likewise. >> (basic_string(const basic_string&, const _Alloc&)): >> Define even if !_GLIBCXX_USE_CXX11_ABI. >> (basic_string(basic_string&&, const _Alloc&)): Likewise >> and add noexcept qualification. >> (basic_string<>::erase): Adapt to take __const_iterator. >> * testsuite/21_strings/basic_string/hash/debug.cc: New test. >> * >> testsuite/21_strings/basic_string/hash/debug_char8_t.cc: New test. >> * >> testsuite/21_strings/basic_string/requirements/citerators.cc: Adapt >> to test __gnu_debug::string >> when _GLIBCXX_DEBUG is defined. >> * >> testsuite/21_strings/basic_string/requirements/dr438/constructor.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/exception/basic.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/exception/generation_prohibited.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/exception/propagation_consistent.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/explicit_instantiation/char/1.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/explicit_instantiation/char16_t/1.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/explicit_instantiation/char32_t/1.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/explicit_instantiation/char8_t/1.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/explicit_instantiation/wchar_t/1.cc: >> Likewise. >> * >> testsuite/21_strings/basic_string/requirements/typedefs.cc: Likewise. >> * testsuite/util/exception/safety.h >> (erase_base<__gnu_debug::basic_string<>>): New partial >> specialization. >> (insert_base<__gnu_debug::basic_string<>>): Likewise. >> * testsuite/util/testsuite_container_traits.h >> (traits<__gnu_debug::basic_string<>>): Likewise. >> >> >> Tested under Linux x86_64. >> >> Ok to commit ? >> >> François >> > >> diff --git a/libstdc++-v3/include/debug/string >> b/libstdc++-v3/include/debug/string >> index 172179530aa..f665c759557 100644 >> --- a/libstdc++-v3/include/debug/string >> +++ b/libstdc++-v3/include/debug/string >> @@ -41,6 +41,14 @@ >> __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func) \ >> ._M_message(#_Cond)._M_error() >> >> +#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 >> +# define _GLIBCXX_INSERT_RETURNS_ITERATOR 1 >> +# define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) expr >> +#else >> +# define _GLIBCXX_INSERT_RETURNS_ITERATOR 0 >> +# define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) >> +#endif >> + >> namespace __gnu_debug >> { >> /** Checks that __s is non-NULL or __n == 0, and then returns __s. */ >> @@ -123,21 +131,21 @@ namespace __gnu_debug >> >> using _Base::npos; >> >> - basic_string() >> - >> _GLIBCXX_NOEXCEPT_IF(std::is_nothrow_default_constructible<_Base>::value) >> - : _Base() { } >> - >> // 21.3.1 construct/copy/destroy: >> + >> explicit >> basic_string(const _Allocator& __a) _GLIBCXX_NOEXCEPT >> : _Base(__a) { } >> >> #if __cplusplus < 201103L >> + basic_string() : _Base() { } >> + >> basic_string(const basic_string& __str) >> : _Base(__str) { } >> >> ~basic_string() { } >> #else >> + basic_string() = default; >> basic_string(const basic_string&) = default; >> basic_string(basic_string&&) = default; >> >> @@ -146,13 +154,15 @@ namespace __gnu_debug >> : _Base(__l, __a) >> { } >> >> -#if _GLIBCXX_USE_CXX11_ABI >> basic_string(const basic_string& __s, const _Allocator& __a) >> : _Base(__s, __a) { } >> >> basic_string(basic_string&& __s, const _Allocator& __a) >> - : _Base(std::move(__s), __a) { } >> -#endif >> + noexcept( noexcept( >> + _Base(std::declval<_Base>()), std::declval<const _Allocator&>()) ) > > There is a closing ')' in the wrong place here. This checks whether > _Base is nothrow_move_constructible and whether std::declval is > nothrow. Well spotted and fixed in this patch. The only problem left is that it is a copy/paste of __gnu_debug::vector implementation. I'll submit a patch for this and maybe for other debug containers that are just missing their noexception qualification. > > You could just use > noexcept(is_nothrow_constructible<_Base, _Base, const > _Allocator&>::value) > or: > noexcept(noexcept(_Base(static_cast<_Base&&>(__s), __a))) > > It's a bit confusing that we have a noexcept specifier that only > depends on _Base when the _Safe base class can also throw: > >> + : _Safe(std::move(__s._M_safe()), __a), >> + _Base(std::move(__s._M_base()), __a) >> + { } > > In fact, it is conditionally noexcept with the same condition as the > _Base type, so checking if the _Base construction is non-throwing is > correct. But confusing. > >> + /// std::hash specialization for __gnu_debug::basic_string. >> + template<typename _CharT> >> + struct hash<__gnu_debug::basic_string<_CharT>> >> + : public __hash_base<size_t, __gnu_debug::basic_string<_CharT>> >> + { >> + size_t >> + operator()(const __gnu_debug::basic_string<_CharT>& __s) const >> noexcept >> + { return std::hash<std::basic_string<_CharT>>()(__s); } >> + }; >> + >> + template<typename _CharT> >> + struct __is_fast_hash<hash<__gnu_debug::basic_string<_CharT>>> >> + : std::false_type > > This says it's always a slow hash, rather than deferring to > __is_fast_hash<hash<std::basic_string<C>>>. > > That means __is_fast_hash is false for __gnu_debug::basic_string<int> > but true for std::basic_string<int>. In practice it probably doesn't > matter, but it's inconsistent. > >> + { }; >> + >> +_GLIBCXX_END_NAMESPACE_VERSION >> +} >> +#endif /* C++11 */ >> + >> +#undef _GLIBCXX_INSERT_RETURNS_ITERATOR >> +#undef _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY >> + >> #endif >> diff --git >> a/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >> b/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >> new file mode 100644 >> index 00000000000..84c989590b7 >> --- /dev/null >> +++ b/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >> @@ -0,0 +1,53 @@ >> +// Copyright (C) 2021 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/>. >> + >> +// { dg-options "-std=gnu++17" } >> +// { dg-do run { target c++17 } } > > -std=gnu++17 is the default now, so should not be given explicitly. > > You could combine this test and debug/hash_char8_t.cc by adding the > char8_t parts guarded by _GLIBCXX_USE_CHAR8_T. When the test is run > with a -std=gnu++20 it will test the char8_t parts (which is why we > don't want the redundant -std=gnu++17, because it would prevent it > from being run with -std=gnu++20). > > >> diff --git >> a/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >> b/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >> >> index 99bf5726dcc..69d4a8d0e51 100644 >> --- >> a/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >> +++ >> b/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >> @@ -18,14 +18,21 @@ >> // with this library; see the file COPYING3. If not see >> // <http://www.gnu.org/licenses/>. >> >> -#include <string> >> #include <testsuite_containers.h> >> >> +#ifdef _GLIBCXX_DEBUG >> +# include <debug/string> >> +using namespace __gnu_debug; >> +#else >> +# include <string> >> +using namespace std; >> +#endif > > This has the same problem I pointed out previously. With this change, > we don't run this test for std::string in debug mode. When debug > mode is active, we test a different type not std::string. > > That means if we introduce a bug to std::string that only affects > debug mode, we might not notice, because we're stopped testing > std::string in debug mode. > > If you want to test __gnu_debug::basic_string then those tests should > be added as new tests, not by replacing existing tests that are > already testing something different. > So I added tests on __gnu_debug::basic_string along with the ones on std::basic_string. The nice effect of this is that it found a bug in exception safety testsuite utils, we could be trying to erase the past-the-end iterator. I still need to find out why, when running test on __gnu_debug::basic_string after the std::basic_string one, the generate(sz) call always returns sz. Tested under Linux x86_64. Ok to commit ? François
On 24/03/21 22:48 +0100, François Dumont wrote: >On 23/03/21 4:42 pm, Jonathan Wakely wrote: >>On 20/03/21 22:32 +0100, François Dumont wrote: >>>Following your feedback here is the simplified version. I grouped >>>it with the patch I submitted before. >>> >>> >>>On 19/03/21 8:41 pm, Jonathan Wakely wrote: >>>>I think we could just define on partial specialization that works for >>>>all cases: >>> >>>Yes, sounds better. But I relied on std::__hash_base which gives >>>directly the correct definition. >> >>But that is wrong. >> >>The requirements in https://wg21.link/unord.hash say that std::hash<T> >>must be disabled for unsupported types. With std::basic_string the >>specialization std::hash<basic_string<int>> is disabled, because there >>is no specialization for that type, so it uses the primary template of >>std::hash, which is empty: >> >> /// Primary class template hash, usable for enum types only. >> // Use with non-enum types still SFINAES. >> template<typename _Tp> >> struct hash : __hash_enum<_Tp> >> { }; >> >>With your patch, std::hash<__gnu_debug::basic_string<int>> will not be >>empty. It will provide argument_type and result_type and operator() >>but calling operator() will fail to compile. >> >>My suggestion doesn't have that problem. With my suggestion, >>hash<_gnu_debug::basic_string<C>> is enabled if (and only if) >>hash<std::basic_string<C>> is enabled. > >Ok, I adopted your approach then. > > >> >>> libstdc++: Fix and complete __gnu_debug::basic_string implementation >>> >>> Fix and complete __gnu_debug::basic_string so that it can be >>>used as a transparent >>> replacement of std::basic_string. >>> >>> libstdc++-v3/ChangeLog: >>> >>> * include/debug/string >>> (basic_string(const _CharT*, const _Allocator&)): >>>Remove assign call. >>> (basic_string<>::insert(const_iterator, _InputIte, >>>_InputIte)): Try to >>> remove iterator debug layer even if !_GLIBCXX_USE_CXX11_ABI. >>> [_GLIBCXX_USE_CHAR8_T] (__gnu_debug::u8string): New. >>> (__gnu_debug::u16string, __gnu_debug::u32string): New. >>>(std::hash<__gnu_debug::basic_string<>>): New partial specialization. >>>(std::__is_fast_hash<__gnu_debug::basic_string<>>): Likewise. >>> (basic_string(const basic_string&, const _Alloc&)): >>>Define even if !_GLIBCXX_USE_CXX11_ABI. >>> (basic_string(basic_string&&, const _Alloc&)): >>>Likewise and add noexcept qualification. >>> (basic_string<>::erase): Adapt to take __const_iterator. >>> * testsuite/21_strings/basic_string/hash/debug.cc: New test. >>> * >>>testsuite/21_strings/basic_string/hash/debug_char8_t.cc: New test. >>> * >>>testsuite/21_strings/basic_string/requirements/citerators.cc: >>>Adapt to test __gnu_debug::string >>> when _GLIBCXX_DEBUG is defined. >>> * >>>testsuite/21_strings/basic_string/requirements/dr438/constructor.cc: >>>Likewise. >>> * >>>testsuite/21_strings/basic_string/requirements/exception/basic.cc: >>>Likewise. >>> * testsuite/21_strings/basic_string/requirements/exception/generation_prohibited.cc: >>>Likewise. >>> * testsuite/21_strings/basic_string/requirements/exception/propagation_consistent.cc: >>>Likewise. >>> * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char/1.cc: >>>Likewise. >>> * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char16_t/1.cc: >>>Likewise. >>> * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char32_t/1.cc: >>>Likewise. >>> * testsuite/21_strings/basic_string/requirements/explicit_instantiation/char8_t/1.cc: >>>Likewise. >>> * testsuite/21_strings/basic_string/requirements/explicit_instantiation/wchar_t/1.cc: >>>Likewise. >>> * >>>testsuite/21_strings/basic_string/requirements/typedefs.cc: >>>Likewise. >>> * testsuite/util/exception/safety.h >>>(erase_base<__gnu_debug::basic_string<>>): New partial >>> specialization. >>>(insert_base<__gnu_debug::basic_string<>>): Likewise. >>> * testsuite/util/testsuite_container_traits.h >>>(traits<__gnu_debug::basic_string<>>): Likewise. >>> >>> >>>Tested under Linux x86_64. >>> >>>Ok to commit ? >>> >>>François >>> >> >>>diff --git a/libstdc++-v3/include/debug/string >>>b/libstdc++-v3/include/debug/string >>>index 172179530aa..f665c759557 100644 >>>--- a/libstdc++-v3/include/debug/string >>>+++ b/libstdc++-v3/include/debug/string >>>@@ -41,6 +41,14 @@ >>> __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func) \ >>> ._M_message(#_Cond)._M_error() >>> >>>+#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 >>>+# define _GLIBCXX_INSERT_RETURNS_ITERATOR 1 >>>+# define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) expr >>>+#else >>>+# define _GLIBCXX_INSERT_RETURNS_ITERATOR 0 >>>+# define _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY(expr) >>>+#endif >>>+ >>>namespace __gnu_debug >>>{ >>> /** Checks that __s is non-NULL or __n == 0, and then returns __s. */ >>>@@ -123,21 +131,21 @@ namespace __gnu_debug >>> >>> using _Base::npos; >>> >>>- basic_string() >>>- _GLIBCXX_NOEXCEPT_IF(std::is_nothrow_default_constructible<_Base>::value) >>>- : _Base() { } >>>- >>> // 21.3.1 construct/copy/destroy: >>>+ >>> explicit >>> basic_string(const _Allocator& __a) _GLIBCXX_NOEXCEPT >>> : _Base(__a) { } >>> >>>#if __cplusplus < 201103L >>>+ basic_string() : _Base() { } >>>+ >>> basic_string(const basic_string& __str) >>> : _Base(__str) { } >>> >>> ~basic_string() { } >>>#else >>>+ basic_string() = default; >>> basic_string(const basic_string&) = default; >>> basic_string(basic_string&&) = default; >>> >>>@@ -146,13 +154,15 @@ namespace __gnu_debug >>> : _Base(__l, __a) >>> { } >>> >>>-#if _GLIBCXX_USE_CXX11_ABI >>> basic_string(const basic_string& __s, const _Allocator& __a) >>> : _Base(__s, __a) { } >>> >>> basic_string(basic_string&& __s, const _Allocator& __a) >>>- : _Base(std::move(__s), __a) { } >>>-#endif >>>+ noexcept( noexcept( >>>+ _Base(std::declval<_Base>()), std::declval<const _Allocator&>()) ) >> >>There is a closing ')' in the wrong place here. This checks whether >>_Base is nothrow_move_constructible and whether std::declval is >>nothrow. > >Well spotted and fixed in this patch. > >The only problem left is that it is a copy/paste of >__gnu_debug::vector implementation. I'll submit a patch for this and >maybe for other debug containers that are just missing their >noexception qualification. > > >> >>You could just use >> noexcept(is_nothrow_constructible<_Base, _Base, const >>_Allocator&>::value) >>or: >> noexcept(noexcept(_Base(static_cast<_Base&&>(__s), __a))) >> >>It's a bit confusing that we have a noexcept specifier that only >>depends on _Base when the _Safe base class can also throw: >> >>>+ : _Safe(std::move(__s._M_safe()), __a), >>>+ _Base(std::move(__s._M_base()), __a) >>>+ { } >> >>In fact, it is conditionally noexcept with the same condition as the >>_Base type, so checking if the _Base construction is non-throwing is >>correct. But confusing. >> >>>+ /// std::hash specialization for __gnu_debug::basic_string. >>>+ template<typename _CharT> >>>+ struct hash<__gnu_debug::basic_string<_CharT>> >>>+ : public __hash_base<size_t, __gnu_debug::basic_string<_CharT>> >>>+ { >>>+ size_t >>>+ operator()(const __gnu_debug::basic_string<_CharT>& __s) >>>const noexcept >>>+ { return std::hash<std::basic_string<_CharT>>()(__s); } >>>+ }; >>>+ >>>+ template<typename _CharT> >>>+ struct __is_fast_hash<hash<__gnu_debug::basic_string<_CharT>>> >>>+ : std::false_type >> >>This says it's always a slow hash, rather than deferring to >>__is_fast_hash<hash<std::basic_string<C>>>. >> >>That means __is_fast_hash is false for __gnu_debug::basic_string<int> >>but true for std::basic_string<int>. In practice it probably doesn't >>matter, but it's inconsistent. >> >>>+ { }; >>>+ >>>+_GLIBCXX_END_NAMESPACE_VERSION >>>+} >>>+#endif /* C++11 */ >>>+ >>>+#undef _GLIBCXX_INSERT_RETURNS_ITERATOR >>>+#undef _GLIBCXX_INSERT_RETURNS_ITERATOR_ONLY >>>+ >>>#endif >>>diff --git >>>a/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >>>b/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >>>new file mode 100644 >>>index 00000000000..84c989590b7 >>>--- /dev/null >>>+++ b/libstdc++-v3/testsuite/21_strings/basic_string/hash/debug.cc >>>@@ -0,0 +1,53 @@ >>>+// Copyright (C) 2021 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/>. >>>+ >>>+// { dg-options "-std=gnu++17" } >>>+// { dg-do run { target c++17 } } >> >>-std=gnu++17 is the default now, so should not be given explicitly. >> >>You could combine this test and debug/hash_char8_t.cc by adding the >>char8_t parts guarded by _GLIBCXX_USE_CHAR8_T. When the test is run >>with a -std=gnu++20 it will test the char8_t parts (which is why we >>don't want the redundant -std=gnu++17, because it would prevent it >>from being run with -std=gnu++20). >> >> >>>diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc b/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >>> >>>index 99bf5726dcc..69d4a8d0e51 100644 >>>--- a/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >>>+++ b/libstdc++-v3/testsuite/21_strings/basic_string/requirements/citerators.cc >>>@@ -18,14 +18,21 @@ >>>// with this library; see the file COPYING3. If not see >>>// <http://www.gnu.org/licenses/>. >>> >>>-#include <string> >>>#include <testsuite_containers.h> >>> >>>+#ifdef _GLIBCXX_DEBUG >>>+# include <debug/string> >>>+using namespace __gnu_debug; >>>+#else >>>+# include <string> >>>+using namespace std; >>>+#endif >> >>This has the same problem I pointed out previously. With this change, >>we don't run this test for std::string in debug mode. When debug >>mode is active, we test a different type not std::string. >> >>That means if we introduce a bug to std::string that only affects >>debug mode, we might not notice, because we're stopped testing >>std::string in debug mode. >> >>If you want to test __gnu_debug::basic_string then those tests should >>be added as new tests, not by replacing existing tests that are >>already testing something different. >> >So I added tests on __gnu_debug::basic_string along with the ones on >std::basic_string. Oh, I like this appraoch. Thanks. >The nice effect of this is that it found a bug in exception safety >testsuite utils, we could be trying to erase the past-the-end >iterator. Nice. >I still need to find out why, when running test on >__gnu_debug::basic_string after the std::basic_string one, the >generate(sz) call always returns sz. The "random" generator will always return the same sequence of numbers every time you run the test. It uses a default-constructed std::mt19937 without a seed, so the sequence of random numbers is 100% reproducable. >Tested under Linux x86_64. > >Ok to commit ? OK thanks.
On 25/03/21 13:05 +0000, Jonathan Wakely wrote: >On 24/03/21 22:48 +0100, François Dumont wrote: >>I still need to find out why, when running test on >>__gnu_debug::basic_string after the std::basic_string one, the >>generate(sz) call always returns sz. > >The "random" generator will always return the same sequence of numbers >every time you run the test. It uses a default-constructed >std::mt19937 without a seed, so the sequence of random numbers is 100% >reproducable. This patch allows those random engines to be seeded, so that we can test with different random numbers. It's already found a bug: GLIBCXX_SEED_TEST_RNG=-941908610 make check RUNTESTFLAGS=conformance.exp=23_containers/forward_list/requirements/exception/generation_prohibited.cc Using random seed 3353058686 FAIL: 23_containers/forward_list/requirements/exception/generation_prohibited.cc execution test We need to investigate that. Any objections to pushing this?
On 25/03/21 14:48 +0000, Jonathan Wakely wrote: >On 25/03/21 13:05 +0000, Jonathan Wakely wrote: >>On 24/03/21 22:48 +0100, François Dumont wrote: >>>I still need to find out why, when running test on >>>__gnu_debug::basic_string after the std::basic_string one, the >>>generate(sz) call always returns sz. >> >>The "random" generator will always return the same sequence of numbers >>every time you run the test. It uses a default-constructed >>std::mt19937 without a seed, so the sequence of random numbers is 100% >>reproducable. > >This patch allows those random engines to be seeded, so that we can test >with different random numbers. > >It's already found a bug: > >GLIBCXX_SEED_TEST_RNG=-941908610 make check RUNTESTFLAGS=conformance.exp=23_containers/forward_list/requirements/exception/generation_prohibited.cc > >Using random seed 3353058686 >FAIL: 23_containers/forward_list/requirements/exception/generation_prohibited.cc execution test > >We need to investigate that. Oh, it's the same generate(sz) bug as you already found. But I've found other bugs, e.g. with GLIBCXX_SEED_TEST_RNG=1908970375). I think we should also add a check for non-empty containers to those test functions, and ensure we don't try to erase from empty containers (see attached).
On 25/03/21 4:29 pm, Jonathan Wakely wrote: > On 25/03/21 14:48 +0000, Jonathan Wakely wrote: >> On 25/03/21 13:05 +0000, Jonathan Wakely wrote: >>> On 24/03/21 22:48 +0100, François Dumont wrote: >>>> I still need to find out why, when running test on >>>> __gnu_debug::basic_string after the std::basic_string one, the >>>> generate(sz) call always returns sz. >>> >>> The "random" generator will always return the same sequence of numbers >>> every time you run the test. It uses a default-constructed >>> std::mt19937 without a seed, so the sequence of random numbers is 100% >>> reproducable. >> >> This patch allows those random engines to be seeded, so that we can test >> with different random numbers. >> >> It's already found a bug: >> >> GLIBCXX_SEED_TEST_RNG=-941908610 make check >> RUNTESTFLAGS=conformance.exp=23_containers/forward_list/requirements/exception/generation_prohibited.cc >> >> Using random seed 3353058686 >> FAIL: >> 23_containers/forward_list/requirements/exception/generation_prohibited.cc >> execution test >> >> We need to investigate that. > > Oh, it's the same generate(sz) bug as you already found. But I've > found other bugs, e.g. with GLIBCXX_SEED_TEST_RNG=1908970375). > > I think we should also add a check for non-empty containers to those > test functions, and ensure we don't try to erase from empty > containers (see attached). > > Yes, I also realized this empty container potential issue. Please go ahead with any of your patches, I'll just adapt if you push first. I will commit in a couple of hours. François
On 25/03/21 18:45 +0100, François Dumont via Libstdc++ wrote: >On 25/03/21 4:29 pm, Jonathan Wakely wrote: >>On 25/03/21 14:48 +0000, Jonathan Wakely wrote: >>>On 25/03/21 13:05 +0000, Jonathan Wakely wrote: >>>>On 24/03/21 22:48 +0100, François Dumont wrote: >>>>>I still need to find out why, when running test on >>>>>__gnu_debug::basic_string after the std::basic_string one, the >>>>>generate(sz) call always returns sz. >>>> >>>>The "random" generator will always return the same sequence of numbers >>>>every time you run the test. It uses a default-constructed >>>>std::mt19937 without a seed, so the sequence of random numbers is 100% >>>>reproducable. >>> >>>This patch allows those random engines to be seeded, so that we can test >>>with different random numbers. >>> >>>It's already found a bug: >>> >>>GLIBCXX_SEED_TEST_RNG=-941908610 make check RUNTESTFLAGS=conformance.exp=23_containers/forward_list/requirements/exception/generation_prohibited.cc >>> >>>Using random seed 3353058686 >>>FAIL: 23_containers/forward_list/requirements/exception/generation_prohibited.cc >>>execution test >>> >>>We need to investigate that. >> >>Oh, it's the same generate(sz) bug as you already found. But I've >>found other bugs, e.g. with GLIBCXX_SEED_TEST_RNG=1908970375). >> >>I think we should also add a check for non-empty containers to those >>test functions, and ensure we don't try to erase from empty >>containers (see attached). >> >> >Yes, I also realized this empty container potential issue. > >Please go ahead with any of your patches, I'll just adapt if you push first. OK, thanks. I've pushed the attached patch. You should only need to undo the generate(sz) changes in your patch. >I will commit in a couple of hours. Great, thanks.
On 25/03/21 4:29 pm, Jonathan Wakely wrote: > Oh, it's the same generate(sz) bug as you already found. But I've > found other bugs, e.g. with GLIBCXX_SEED_TEST_RNG=1908970375). > > I think we should also add a check for non-empty containers to those > test functions, and ensure we don't try to erase from empty > containers (see attached). > > I had a look at this patch and on my side I was more thinking about avoiding empty containers in the first place. What do you think ? François
On 29/03/21 22:25 +0200, François Dumont wrote: >On 25/03/21 4:29 pm, Jonathan Wakely wrote: >>Oh, it's the same generate(sz) bug as you already found. But I've >>found other bugs, e.g. with GLIBCXX_SEED_TEST_RNG=1908970375). >> >>I think we should also add a check for non-empty containers to those >>test functions, and ensure we don't try to erase from empty >>containers (see attached). >> >> >I had a look at this patch and on my side I was more thinking about >avoiding empty containers in the first place. > >What do you think ? I did consider this and decided against it for the reasons below. >François > >diff --git a/libstdc++-v3/testsuite/util/exception/safety.h b/libstdc++-v3/testsuite/util/exception/safety.h >index 54449d2f7bb..6940f2f765d 100644 >--- a/libstdc++-v3/testsuite/util/exception/safety.h >+++ b/libstdc++-v3/testsuite/util/exception/safety.h >@@ -55,14 +55,14 @@ namespace __gnu_test > > // Return randomly generated integer on range [0, __max_size]. > static size_type >- generate(size_type __max_size) >+ generate(size_type __max_size, size_type __min_size = 0) > { > using param_type = typename distribution_type::param_type; > > // Make the engine and distribution static... > static engine_type engine = get_engine(); > static distribution_type distribution; >- return distribution(engine, param_type{0, __max_size}); >+ return distribution(engine, param_type{__min_size, __max_size}); > } > > // Given an instantiating type, return a unique value. >@@ -198,7 +198,8 @@ namespace __gnu_test > > // Size test container. > const size_type max_elements = 100; >- size_type n = generate(max_elements); >+ const size_type min_elements = 10; >+ size_type n = generate(max_elements, min_elements); Why 10 though? Why not 1? Otherwise we never test the case where you erase a single element from a container that only has a single element. And it would also mean we never test inserting into an empty container, because it always starts with at least 10 elements. So I decided that it was better to keep /most/ of the tests unchanged, so they sometimes (depending on the RNG, which can now be made less predictable by using a non-default seed) test with empty containers. Only the operations that really can't work for empty containers need to care about them. For other operations, we should keep testing the empty case.
diff --git a/libstdc++-v3/include/debug/string b/libstdc++-v3/include/debug/string index d6eb5280ade..dec23f6277b 100644 --- a/libstdc++-v3/include/debug/string +++ b/libstdc++-v3/include/debug/string @@ -41,6 +41,14 @@ __gnu_debug::_Error_formatter::_S_at(_File, _Line, _Func) \ ._M_message(#_Cond)._M_error() +#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 +# define _GLIBCXX_CPP11_AND_CXX11_ABI 1 +# define _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY(Statement) Statement +#else +# define _GLIBCXX_CPP11_AND_CXX11_ABI 0 +# define _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY(Statement) +#endif + namespace __gnu_debug { /** Checks that __s is non-NULL or __n == 0, and then returns __s. */ @@ -180,7 +188,7 @@ namespace __gnu_debug basic_string(const _CharT* __s, const _Allocator& __a = _Allocator()) : _Base(__glibcxx_check_string_constructor(__s), __a) - { this->assign(__s); } + { } basic_string(size_type __n, _CharT __c, const _Allocator& __a = _Allocator()) @@ -637,15 +645,22 @@ namespace __gnu_debug __glibcxx_check_insert_range(__p, __first, __last, __dist); typename _Base::iterator __res; -#if _GLIBCXX_USE_CXX11_ABI && __cplusplus >= 201103 +#if ! _GLIBCXX_CPP11_AND_CXX11_ABI + const size_type __offset = __p.base() - _Base::begin(); +#endif if (__dist.second >= __dp_sign) - __res = _Base::insert(__p.base(), __gnu_debug::__unsafe(__first), - __gnu_debug::__unsafe(__last)); + { + _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY(__res =) + _Base::insert(__p.base(), __gnu_debug::__unsafe(__first), + __gnu_debug::__unsafe(__last)); + } else - __res = _Base::insert(__p.base(), __first, __last); -#else - const size_type __offset = __p.base() - _Base::begin(); - _Base::insert(__p.base(), __first, __last); + { + _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY(__res =) + _Base::insert(__p.base(), __first, __last); + } + +#if ! _GLIBCXX_CPP11_AND_CXX11_ABI __res = _Base::begin() + __offset; #endif this->_M_invalidate_all(); @@ -1287,6 +1302,19 @@ namespace __gnu_debug typedef basic_string<wchar_t> wstring; #endif +#ifdef _GLIBCXX_USE_CHAR8_T + /// A string of @c char8_t + typedef basic_string<char8_t> u8string; +#endif + +#if __cplusplus >= 201103L + /// A string of @c char16_t + typedef basic_string<char16_t> u16string; + + /// A string of @c char32_t + typedef basic_string<char32_t> u32string; +#endif + template<typename _CharT, typename _Traits, typename _Allocator> struct _Insert_range_from_self_is_safe< __gnu_debug::basic_string<_CharT, _Traits, _Allocator> > @@ -1294,4 +1322,96 @@ namespace __gnu_debug } // namespace __gnu_debug +#if __cplusplus >= 201103L + +namespace std _GLIBCXX_VISIBILITY(default) +{ +_GLIBCXX_BEGIN_NAMESPACE_VERSION + + // DR 1182. + +#ifndef _GLIBCXX_COMPATIBILITY_CXX0X + /// std::hash specialization for string. + template<> + struct hash<__gnu_debug::string> + : public __hash_base<size_t, __gnu_debug::string> + { + size_t + operator()(const __gnu_debug::string& __s) const noexcept + { return std::hash<std::string>()(__s); } + }; + + template<> + struct __is_fast_hash<hash<__gnu_debug::string>> : std::false_type + { }; + +#ifdef _GLIBCXX_USE_WCHAR_T + /// std::hash specialization for wstring. + template<> + struct hash<__gnu_debug::wstring> + : public __hash_base<size_t, __gnu_debug::wstring> + { + size_t + operator()(const __gnu_debug::wstring& __s) const noexcept + { return std::hash<__gnu_debug::wstring>()(__s); } + }; + + template<> + struct __is_fast_hash<hash<__gnu_debug::wstring>> : std::false_type + { }; +#endif +#endif /* _GLIBCXX_COMPATIBILITY_CXX0X */ + +#ifdef _GLIBCXX_USE_CHAR8_T + /// std::hash specialization for u8string. + template<> + struct hash<__gnu_debug::u8string> + : public __hash_base<size_t, __gnu_debug::u8string> + { + size_t + operator()(const __gnu_debug::u8string& __s) const noexcept + { return std::hash<std::u8string>()(__s); } + }; + + template<> + struct __is_fast_hash<hash<__gnu_debug::u8string>> : std::false_type + { }; +#endif + + /// std::hash specialization for u16string. + template<> + struct hash<__gnu_debug::u16string> + : public __hash_base<size_t, __gnu_debug::u16string> + { + size_t + operator()(const __gnu_debug::u16string& __s) const noexcept + { return std::hash<std::u16string>()(__s); } + }; + + template<> + struct __is_fast_hash<hash<__gnu_debug::u16string>> : std::false_type + { }; + + /// std::hash specialization for u32string. + template<> + struct hash<__gnu_debug::u32string> + : public __hash_base<size_t, __gnu_debug::u32string> + { + size_t + operator()(const __gnu_debug::u32string& __s) const noexcept + { return std::hash<std::u32string>()(__s); } + }; + + template<> + struct __is_fast_hash<hash<__gnu_debug::u32string>> : std::false_type + { }; + +_GLIBCXX_END_NAMESPACE_VERSION + } // namespace std + +#endif /* C++11 */ + +#undef _GLIBCXX_CPP11_AND_CXX11_ABI +#undef _GLIBCXX_CPP11_AND_CXX11_ABI_ONLY + #endif diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc b/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc index c0e8975ce4a..7f8792bbd76 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/hash/hash_char8_t.cc @@ -18,10 +18,19 @@ // { dg-options "-std=gnu++2a" } // { dg-do run { target c++2a } } -#include <string> #include <memory_resource> #include <testsuite_hooks.h> +#ifdef _GLIBCXX_DEBUG +# include <debug/string> + +using namespace __gnu_debug; +#else +# include <string> + +using namespace std; +#endif + // C++2a N4810 21.3.5 [basic.string.hash] // If S is one of these string types, SV is the corresponding string view type, // and s is an object of type S, then hash<S>()(s) == hash<SV>()(SV(s)). @@ -38,8 +47,8 @@ template<typename S> void test01() { - VERIFY( test(std::string("a narrow string")) ); - VERIFY( test(std::u8string(u8"a utf-8 string")) ); + VERIFY( test(string("a narrow string")) ); + VERIFY( test(u8string(u8"a utf-8 string")) ); #if _GLIBCXX_USE_CXX11_ABI VERIFY( test(std::pmr::string("a narrow string, but with PMR!")) ); VERIFY( test(std::pmr::u8string(u8"a utf-8 string, but with PMR!")) ); @@ -50,9 +59,9 @@ void test02() { using std::hash; - std::string native("a string, a string, my stringdom for a string"); - std::u8string utf8(u8"a string, a string, my stringdom for a string"); - VERIFY( hash<std::string>()(native) == hash<std::u8string>()(utf8) ); + string native("a string, a string, my stringdom for a string"); + u8string utf8(u8"a string, a string, my stringdom for a string"); + VERIFY( hash<string>()(native) == hash<u8string>()(utf8) ); } int