Re: [PATCH] Complete __gnu_debug::basic_string

[Jonathan Wakely <jwakely@redhat.com>]

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.