Re: [PATCH] Complete __gnu_debug::basic_string

[Jonathan Wakely <jwakely@redhat.com>]

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.