[PATCH] Complete __gnu_debug::basic_string
Jonathan Wakely
jwakely@redhat.com
Tue Mar 23 15:42:36 GMT 2021
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.
More information about the Libstdc++
mailing list