Re: [PATCH v2 3/5] Add gdb::string_view

[Pedro Alves <palves at redhat dot com>]

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