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 > must be disabled for unsupported types. With std::basic_string the > specialization std::hash> 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 >     struct hash : __hash_enum<_Tp> >     { }; > > With your patch, std::hash<__gnu_debug::basic_string> 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> is enabled if (and only if) > hash> 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()) ) > > 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 >> +    struct hash<__gnu_debug::basic_string<_CharT>> >> +    : public __hash_base> >> +    { >> +      size_t >> +      operator()(const __gnu_debug::basic_string<_CharT>& __s) const >> noexcept >> +      { return std::hash>()(__s); } >> +    }; >> + >> +  template >> +    struct __is_fast_hash>> >> +    : std::false_type > > This says it's always a slow hash, rather than deferring to > __is_fast_hash>>. > > That means __is_fast_hash is false for __gnu_debug::basic_string > but true for std::basic_string. 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 >> +// . >> + >> +// { 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 >> // . >> >> -#include >> #include >> >> +#ifdef _GLIBCXX_DEBUG >> +# include >> +using namespace __gnu_debug; >> +#else >> +# include >> +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