[libcxx] r276238 - Implement std::string_view as described in http://wg21.link/P0254R1. Reviewed as https://reviews.llvm.org/D21459
Duncan P. N. Exon Smith via cfe-commits
cfe-commits at lists.llvm.org
Sun Jun 18 11:05:15 PDT 2017
> On 2017-Jun-18, at 10:46, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>>
>> On 2017-Jun-16, at 05:58, Duncan Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>>
>>
>> On Jun 15, 2017, at 22:22, Eric Fiselier <eric at efcs.ca <mailto:eric at efcs.ca>> wrote:
>>
>>>
>>>
>>> On Thu, Jun 15, 2017 at 11:00 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto: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
>
> FTR, it also breaks code that relies on string_view::clear(), which disappeared.
More importantly, it breaks code that relies on string_view::to_string(). This one matters, since a "true" std::experimental::string_view wouldn't have the implicit conversions.
>
>>>> On Jun 15, 2017, at 21:55, Eric Fiselier <eric at efcs.ca <mailto: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?
>>
>> The type trait restricts it to things convertible to string_view that are not const char *.
>
> I had a bit of a look at experimental/filesystem, and it relies pretty heavily on the string/string_view conversions. I still feel like this approach might be "the right one", but perhaps it's not worth it.
>
>>>> /Eric
>>>>
>>>> On Thu, Jun 15, 2017 at 10:51 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>>>>
>>>>> On Jun 15, 2017, at 19:42, Eric Fiselier <eric at efcs.ca <mailto:eric at efcs.ca>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Thu, Jun 15, 2017 at 8:38 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto: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 <mailto: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 <mailto:dexonsmith at apple.com>>wrote:
>>>>>>
>>>>>> > On Jul 20, 2016, at 22:31, Marshall Clow via cfe-commits <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>> wrote:
>>>>>> >
>>>>>> > Modified: libcxx/trunk/include/string
>>>>>> > URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/string?rev=276238&r1=276237&r2=276238&view=diff <http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/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/20170618/f3f949fd/attachment-0001.html>
More information about the cfe-commits
mailing list