[libcxx] r276238 - Implement std::string_view as described in http://wg21.link/P0254R1. Reviewed as https://reviews.llvm.org/D21459

[Eric Fiselier via cfe-commits]

On Thu, Jun 15, 2017 at 11:00 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> Your suggestion is essentially to replace experimental/string_view with
> something like:
>
>     namespace std { inline namespace __1 { namespace experimental {
>       template <class CharT>
>       using basic_string_view = _VSTD::basic_string_view;
>     }}}
>
> That breaks:
> 1. User compiles 1.cpp with older toolchain.  1.cpp implements
> foo(std::experimental::string_view).
> 2. User compiles 2.cpp with newer toolchain.  2.cpp calls
> foo(std::experimental::string_view).
> 3. User links 1.o with 2.o.
>
> I'm not sure if this matters.
>

It can't matter. <experimental/foo> are allowed to break both their API and
ABI as needed.

Also I was suggesting

   namespace std { namespace experimental {
     using std::basic_string_view;
     using std::string_view;
  }}

This approach will break code that expects experimental::string_view and
std::string_view are different types:
Example:

  void foo(std::string_view);
  void foo(std::experimental::string_view);
  foo(std::string_view{}); // ambiguous


> On Jun 15, 2017, at 21:55, Eric Fiselier <eric at efcs.ca> wrote:
>
> I would also want to do serious performance analysis on this patch. Does
> removing the string_view overloads cause less optimal overloads to be
> chosen? Perhaps allocating ones?
> That would be really unfortunate, and I'm not sure that's in the best
> interest of our users at large.
>
>
> Less optimal compared to what?  C++17 code?
>

Not sure yet, I'm trying to figure out what types the `const Tp&` overloads
are attempting to soak up. Is it only string_view?



>
> /Eric
>
> On Thu, Jun 15, 2017 at 10:51 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
>
>>
>> On Jun 15, 2017, at 19:42, Eric Fiselier <eric at efcs.ca> wrote:
>>
>>
>>
>> On Thu, Jun 15, 2017 at 8:38 PM, Duncan P. N. Exon Smith <
>> dexonsmith at apple.com> wrote:
>>
>>> I just started working on a patch to add #if guards, and the first
>>> interesting thing I found was the basic_string constructor:
>>>
>>> template <class _CharT, class _Traits, class _Allocator>
>>> template <class _Tp>
>>> basic_string<_CharT, _Traits, _Allocator>::basic_string(
>>>              const _Tp& __t, size_type __pos, size_type __n, const
>>> allocator_type& __a,
>>>  typename enable_if<__can_be_converted_to_string_view<_CharT, _Traits,
>>> _Tp>::value, void>::type *)
>>>     : __r_(__second_tag(), __a)
>>> {
>>> __self_view __sv = __self_view(__t).substr(__pos, __n);
>>>     __init(__sv.data(), __sv.size());
>>> #if _LIBCPP_DEBUG_LEVEL >= 2
>>>     __get_db()->__insert_c(this);
>>> #endif
>>> }
>>>
>>>
>>>
>> That constructor was added in C++17, so removing it along with
>> string_view should be OK.
>> Assuming we don't use it to implement older constructors using a single
>> template.
>>
>>
>>
>>> I suppose the decision was made so that std::string could take advantage
>>> of it.
>>>
>>> Is it a conforming extension?
>>>
>>
>> No, because it can change the meaning of otherwise well defined code, as
>> you pointed out initially.
>>
>>
>> Let me know if this patch is along the right lines.  If so, I'll finish
>> it up and put it on phab.
>>
>> experimental/filesystem/path.cpp doesn't compile, since
>> experimental/filesystem uses things like operator+=(string, string_view)
>> extensively.  But I'd like an early opinion on the approach before I dig in.
>>
>> In string, the only function that needed to be rewritten was
>> string::compare(size, size, string, size, size).  I'm nervous that
>> filesystem will be a bigger job.
>>
>>
>>
>>
>>
>>>
>>> On Jun 15, 2017, at 18:35, Eric Fiselier <eric at efcs.ca> wrote:
>>>
>>> It *shouldn't* include <string_view>, that's a given.
>>>
>>> IIRC, and Marshall would know better, I believe it was untenable to
>>> maintain a version of <string> that didn't depend on <string_view> after
>>> making
>>> the changes required for C++17.
>>>
>>> However inspecting <string> now it does seem possible that the
>>> entanglement
>>> is avoidable.Though it's also likely I'm just not seeing the whole
>>> picture.
>>>
>>> /Eric
>>>
>>> On Thu, Jun 15, 2017 at 6:42 PM, Duncan P. N. Exon Smith <
>>> dexonsmith at apple.com>wrote:
>>>
>>>>
>>>> > On Jul 20, 2016, at 22:31, Marshall Clow via cfe-commits <
>>>> cfe-commits at lists.llvm.org> wrote:
>>>> >
>>>> > Modified: libcxx/trunk/include/string
>>>> > URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/includ
>>>> e/string?rev=276238&r1=276237&r2=276238&view=diff
>>>> > ============================================================
>>>> ==================
>>>> >
>>>> > @@ -435,6 +461,7 @@ basic_string<char32_t> operator "" s( co
>>>> > */
>>>> >
>>>> > #include <__config>
>>>> > +#include <string_view>
>>>>
>>>> This breaks the following, valid, C++14 code:
>>>>
>>>>     #include <string>
>>>>     #include <experimental/string_view>
>>>>     using namespace std;
>>>>     using std::experimental::string_view;
>>>>     void f() { string_view sv; }
>>>>
>>>> Should <string> #include <string_view> even when we're not in C++17
>>>> mode?  Why?
>>>>
>>>> > #include <iosfwd>
>>>> > #include <cstring>
>>>> > #include <cstdio>  // For EOF.
>>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170615/cd3588c2/attachment.html>