[PATCH] Complete __gnu_debug::basic_string

Wed Mar 24 21:48:26 GMT 2021

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: debug_string.patch
Type: text/x-patch
Size: 20927 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210324/adf09156/attachment-0001.bin>