From: Pedro Alves <palves@redhat.com>
To: Simon Marchi <simon.marchi@polymtl.ca>, gdb-patches@sourceware.org
Subject: Re: [PATCH v2 3/5] Add gdb::string_view
Date: Fri, 06 Apr 2018 18:08:00 -0000 [thread overview]
Message-ID: <01346239-a0d8-055f-7b6c-86ac48bfc7ac@redhat.com> (raw)
In-Reply-To: <20180330214647.485-3-simon.marchi@polymtl.ca>
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<typename _Tp>
> - using __idt = common_type_t<_Tp>;
> + using __idt = typename std::common_type<_Tp>::type;
> }
>
> template<typename _CharT, typename _Traits>
> - 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<typename _CharT, typename _Traits>
> - 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<char>;
> -#ifdef _GLIBCXX_USE_WCHAR_T
> - using wstring_view = basic_string_view<wchar_t>;
> -#endif
> -#ifdef _GLIBCXX_USE_C99_STDINT_TR1
> - using u16string_view = basic_string_view<char16_t>;
> - using u32string_view = basic_string_view<char32_t>;
> -#endif
>
> // [string.view.hash], hash support:
>
> @@ -565,97 +564,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> template<>
> struct hash<string_view>
> - : public __hash_base<size_t, string_view>
> + : public std::__hash_base<size_t, string_view>
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
next prev parent reply other threads:[~2018-04-06 18:08 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-30 21:47 [PATCH v2 1/5] Update ax_cv_cxx_compile_cxx.m4 Simon Marchi
2018-03-30 21:47 ` [PATCH v2 5/5] Adapt and integrate string_view tests Simon Marchi
2018-04-06 18:20 ` Pedro Alves
2018-03-30 21:47 ` [PATCH v2 4/5] Copy string_view tests from libstdc++ Simon Marchi
2018-03-30 21:47 ` [PATCH v2 2/5] Copy string_view files " Simon Marchi
2018-03-30 21:47 ` [PATCH v2 3/5] Add gdb::string_view Simon Marchi
2018-04-06 18:08 ` Pedro Alves [this message]
2018-04-06 20:37 ` Simon Marchi
2018-04-06 22:01 ` Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=01346239-a0d8-055f-7b6c-86ac48bfc7ac@redhat.com \
--to=palves@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=simon.marchi@polymtl.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).