> On 2017-Jun-18, at 10:46, Duncan P. N. Exon Smith <dexonsm...@apple.com> > wrote: > >> >> On 2017-Jun-16, at 05:58, Duncan Exon Smith <dexonsm...@apple.com >> <mailto:dexonsm...@apple.com>> wrote: >> >> >> On Jun 15, 2017, at 22:22, Eric Fiselier <e...@efcs.ca >> <mailto:e...@efcs.ca>> wrote: >> >>> >>> >>> On Thu, Jun 15, 2017 at 11:00 PM, Duncan P. N. Exon Smith >>> <dexonsm...@apple.com <mailto:dexonsm...@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 <e...@efcs.ca >>>> <mailto:e...@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 >>>> <dexonsm...@apple.com <mailto:dexonsm...@apple.com>> wrote: >>>> >>>>> On Jun 15, 2017, at 19:42, Eric Fiselier <e...@efcs.ca >>>>> <mailto:e...@efcs.ca>> wrote: >>>>> >>>>> >>>>> >>>>> On Thu, Jun 15, 2017 at 8:38 PM, Duncan P. N. Exon Smith >>>>> <dexonsm...@apple.com <mailto:dexonsm...@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 <e...@efcs.ca >>>>>> <mailto:e...@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 >>>>>> <dexonsm...@apple.com <mailto:dexonsm...@apple.com>>wrote: >>>>>> >>>>>> > On Jul 20, 2016, at 22:31, Marshall Clow via cfe-commits >>>>>> > <cfe-commits@lists.llvm.org <mailto:cfe-commits@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.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
