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

[Simon Marchi <simon.marchi@polymtl.ca>]

Thanks for the comments.

On 2018-04-06 14:08, Pedro Alves wrote:
> 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};
>        }


Ah, from the name I thought that experimental version contained the code 
implementing future standards, before they were official... so I'll 
start from scratch using the version from experimental, does that sounds 
good?

>>      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++ ?

Hmm indeed.  Well, it looks like building with clang + libstdcxx fails.  
I'll make sure to test both of these combinations in the next iteration.

Simon