From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 74B413850402 for ; Tue, 23 Mar 2021 15:42:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 74B413850402 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-597-WyNSbHI3Nwqye9GKVISbbA-1; Tue, 23 Mar 2021 11:42:39 -0400 X-MC-Unique: WyNSbHI3Nwqye9GKVISbbA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 453E9104FC01; Tue, 23 Mar 2021 15:42:38 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9C0E05C1C5; Tue, 23 Mar 2021 15:42:37 +0000 (UTC) Date: Tue, 23 Mar 2021 15:42:36 +0000 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: [PATCH] Complete __gnu_debug::basic_string Message-ID: <20210323154236.GX3008@redhat.com> References: <53722923-e31e-4311-27cb-c66d60b52b19@gmail.com> <20210319194115.GU3008@redhat.com> <648191d4-da3b-9e94-67a4-8121a2da489f@gmail.com> MIME-Version: 1.0 In-Reply-To: <648191d4-da3b-9e94-67a4-8121a2da489f@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Mar 2021 15:42:48 -0000 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 >> >>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. >>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()) ) 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 >+ 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.