From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21190 invoked by alias); 6 Apr 2018 18:08:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 21173 invoked by uid 89); 6 Apr 2018 18:08:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD,URIBL_SBL autolearn=no version=3.3.2 spammy=integrate, fundamentals, gccs, Marchi X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Apr 2018 18:08:18 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5E9304068024; Fri, 6 Apr 2018 18:08:17 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id B9C8210F1BED; Fri, 6 Apr 2018 18:08:16 +0000 (UTC) Subject: Re: [PATCH v2 3/5] Add gdb::string_view To: Simon Marchi , gdb-patches@sourceware.org References: <20180330214647.485-1-simon.marchi@polymtl.ca> <20180330214647.485-3-simon.marchi@polymtl.ca> From: Pedro Alves Message-ID: <01346239-a0d8-055f-7b6c-86ac48bfc7ac@redhat.com> Date: Fri, 06 Apr 2018 18:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180330214647.485-3-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-04/txt/msg00097.txt.bz2 On 03/30/2018 10:46 PM, Simon Marchi wrote: > We had a few times the need for a data structure that does essentially > what C++17's std::string_view does, which is to give an std::string-like > interface (only the read-only operations) to an arbitrary character > buffer. > > This patch adapts the files copied from libstdc++ by the previous patch > to integrate them with GDB. Here's a summary of the changes: > > * Remove things related to wstring_view, u16string_view and > u32string_view (I don't think we need them, but we can always add them > later). Yeah, I don't think we'll ever need them. > > * Remove usages of _GLIBCXX_BEGIN_NAMESPACE_VERSION and > _GLIBCXX_END_NAMESPACE_VERSION. > > * Put the code in the gdb namespace. I had to add a few "std::" in > front of std type usages. > > * Add a constructor that builds a string_view from an std::string, so > that we can pass strings to string_view parameters seamlessly. > Normally, that's handled by "operator __sv_type" in the std::string > declaration, but it only exists when building with c++17. Yeah, that's how it was done when string_view was still part of the C++ Library Fundamentals TS: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4480.html#string.view.cons I'd suggest taking a look at the diff between gcc/libstdc++'s: src/libstdc++-v3/include/experimental/string_view src/libstdc++-v3/include/std/string_view > > * Change __throw_out_of_range_fmt() for error(). > > * Make gdb::string_view and alias of std::string_view when building > with >= c++17. s/and/an > > * Remove a bunch of constexpr, because they are not valid in c++11 > (e.g. they are not a single return line). You could instead pick the versions of those functions from the experimental string_view instead, which is written in C++11 constexpr form. E.g.: constexpr basic_string_view - substr(size_type __pos, size_type __n=npos) const + substr(size_type __pos, size_type __n = npos) const noexcept(false) { - return __pos <= this->_M_len - ? basic_string_view{this->_M_str + __pos, - std::min(__n, size_type{this->_M_len - __pos})} - : (__throw_out_of_range_fmt(__N("basic_string_view::substr: __pos " - "(which is %zu) > this->size() " - "(which is %zu)"), - __pos, this->size()), basic_string_view{}); + __pos = _M_check(__pos, "basic_string_view::substr"); + const size_type __rlen = std::min(__n, _M_len - __pos); + return basic_string_view{_M_str + __pos, __rlen}; } > * GCC complains about us defining the ""sv user-defined literal, > because user code is supposed to use suffixes that begin with _. > However, we only use that code with c++ < 17, where we know the sv > literal is not defined. Recent GCCs allow turning off the warning > (-Wliteral-suffix), but gcc 4.8.4 (what the GCC compile farm ARM > builders have) don't have a way to silence this warning. The only way > I found was to add back #pragma GCC system_header, but at least I > could add it at the very end of the file. The pragma only makes the > rest of the file (what follows it) part of a system header, so the > code above should not be considered a system header. > > The string_view class in cli/cli-script.c is removed and its usage > replaced with the new gdb::string_view. Eheh, I knew that day would come when I added that little class. :-) > private: > > - static constexpr int > + static int > _S_compare(size_type __n1, size_type __n2) noexcept > { > const difference_type __diff = __n1 - __n2; > @@ -428,11 +434,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > // argument participates in template argument deduction and the other > // argument gets implicitly converted to the deduced type. See n3766.html. > template > - using __idt = common_type_t<_Tp>; > + using __idt = typename std::common_type<_Tp>::type; > } > > template > - constexpr bool > + bool > operator==(basic_string_view<_CharT, _Traits> __x, > basic_string_view<_CharT, _Traits> __y) noexcept > { return __x.size() == __y.size() && __x.compare(__y) == 0; } > @@ -541,8 +547,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > // [string.view.io], Inserters and extractors > template > - inline basic_ostream<_CharT, _Traits>& > - operator<<(basic_ostream<_CharT, _Traits>& __os, > + inline std::basic_ostream<_CharT, _Traits>& > + operator<<(std::basic_ostream<_CharT, _Traits>& __os, > basic_string_view<_CharT,_Traits> __str) > { return __ostream_insert(__os, __str.data(), __str.size()); } > > @@ -550,13 +556,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > // basic_string_view typedef names > > using string_view = basic_string_view; > -#ifdef _GLIBCXX_USE_WCHAR_T > - using wstring_view = basic_string_view; > -#endif > -#ifdef _GLIBCXX_USE_C99_STDINT_TR1 > - using u16string_view = basic_string_view; > - using u32string_view = basic_string_view; > -#endif > > // [string.view.hash], hash support: > > @@ -565,97 +564,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > template<> > struct hash > - : public __hash_base > + : public std::__hash_base This looks suspiciously like using an internal implementation detail of libstdc++. Where is std::__hash_base defined? Does this compile with clang + libc++ ? Thanks, Pedro Alves