[libc++] Implement P1989R2: range constructor for string_view

[jloser]

Summary

Implement P1989R2 which adds a range constructor for string_view.

Adjust operator/= in path to avoid atomic constraints caching issue
getting provoked from this PR.

Add defaulted template argument to string_view's "sufficient
overloads" to avoid mangling issues in clang-cl builds. It is a
MSVC mangling bug that this works around.

[jloser]

Simplify wrong traits_type test

[• Quuxplusone]

libcxx/include/string_view

291

👍
I idly wonder if it's possible to make a (contrived) test case that regression-tests this. There's https://stackoverflow.com/a/62644127/1424877 but it works only for conversions to user-defined class types, not to size_t.

314

Linebreak before basic_string_view, and perhaps also before : if the line length is still an issue. (Compare line 293.)

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

84

Might as well test const std::vector<char>&& while you're here. (And I'd remove the end-of-line comments; they don't add anything.)

86

Suggest SizedButNotContiguousRange, to match the pattern set by line 90.

109

SGTM.

111–112

Since begin and end are never called, you can omit their bodies.
Personally I'd also mark them const, just to prove that their non-const-ness is not related to the point of this test.

116

Either make WithTraitsType a non-template, or use its templateyness to prove that string_view is constructible with the correct traits type:

#include "constexpr_char_traits.h"  // We have no "really weird char traits" helper header? Darn.
using CCT = constexpr_char_traits<char>;
static_assert(std::is_constructible_v<std::string_view, WithTraitsType<std::char_traits<char>>>);
static_assert(std::is_constructible_v<std::wstring_view, WithTraitsType<std::char_traits<wchar_t>>>);
static_assert(std::is_constructible_v<std::basic_string_view<char, CCT>, WithTraitsType<CCT>>);
static_assert(!std::is_constructible_v<std::string_view, WithTraitsType<CCT>>);  // wrong traits type
static_assert(!std::is_constructible_v<std::wstring_view, WithTraitsType<std::char_traits<char>>>);  // wrong traits type

Here I'm also drive-by testing that we're looking at the whole traits type, not just its character type.

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

8

IIUC, Phab is confused here: you're not actually removing any test coverage here, you're just adding a second copy of this file. Right?

[jloser]

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

8

It's a git mv of the file from libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp to libcxx/test/std/strings/string.view/string.view.deduct/iterator_sentinel.pass.cpp. There is no loss in coverage.

There isn't necessarily precedence for this (having a deduct subdirectory - we usually have a single deduct.pass.cpp). However, I find this pattern a bit nicer when there's several out-of-function-line static_asserts and such. Having it all be comingled in a single deduct.pass.cpp didn't seem as nice. WDYT?

[• Quuxplusone]

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

8

I think this is fine. Just checking that I understood what was mechanically happening. (As you see, Phab is displaying the diff as if you'd added , c++20 to an UNSUPPORTED line, which would obviously be bad if that's actually what you'd done; but Phab is confused by the copy operation and actually this is fine.)

[jloser]

Address most of Arthur's feedback. One test in from_range.pass still passing.

Also still need to dig into GCC11 failing tests.

[jloser]

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

116

I like that idea -- it actually caught a bug in our implementation since I was comparing the traits_type with _Char and not _Traits!. Still need to figure out why our implementation is rejecting

static_assert(std::is_constructible_v<std::wstring_view, WithTraitsType<std::char_traits<wchar_t>>>);

[• Quuxplusone]

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

116

That'll be because if you're keeping WithTraitsType as a template, then you must make it respect its traits type in begin and end.

template<class CharTraits>
struct WithTraitsType {
  typename CharTraits::char_type *begin() const;
  typename CharTraits::char_type *end() const;
  using traits_type = CharTraits;
};

should fix it.

[jloser]

Use char_type from CharTraits in WithTraitsType

[jloser]

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

116

Right, that's it. That skipped right past my brain this morning before coffee. :)

I left the CharTraits as a template.

[jloser]

Make begin() and end() declarations const in from_range.pass.cpp

[jloser]

libcxx/include/string_view

291

Right, it would be nice to test this. I'll think on it some the next few days. It may be addressed in a separate patch to decouple it from this one if I find a way.

[jloser]

I don't yet fully understand why CI is failing some range concepts that were previously true for the type const std::filesystem::path& in a few tests. It's only when the std::string_view constructor is implemented; they work fine without it (since that's just top-of-tree from libc++ library code perspective).

I think one of them is a Clang bug in eager instantiation of constraints in libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp - specifically these below. They all fail, but only when the new std::string_view constructor is implemented.

static_assert(std::same_as<std::ranges::iterator_t<fs::path const>, fs::path::const_iterator>);
static_assert(std::ranges::common_range<fs::path const>);
static_assert(std::ranges::bidirectional_range<fs::path const>);

The GCC11 CI failures are due to a change in behavior with ranges::begin for const std::filesystem::path that I don't yet understand. Possibly a libc++ bug in ranges::begin?

Any ideas, @Quuxplusone @cjdb @ldionne?

[jloser]

In D113161#3129568, @jloser wrote:

I don't yet fully understand why CI is failing some range concepts that were previously true for the type const std::filesystem::path& in a few tests. It's only when the std::string_view constructor is implemented; they work fine without it (since that's just top-of-tree from libc++ library code perspective).

I think one of them is a Clang bug in eager instantiation of constraints in libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp - specifically these below. They all fail, but only when the new std::string_view constructor is implemented.

static_assert(std::same_as<std::ranges::iterator_t<fs::path const>, fs::path::const_iterator>);
static_assert(std::ranges::common_range<fs::path const>);
static_assert(std::ranges::bidirectional_range<fs::path const>);

The GCC11 CI failures are due to a change in behavior with ranges::begin for const std::filesystem::path that I don't yet understand. Possibly a libc++ bug in ranges::begin?

Any ideas, @Quuxplusone @cjdb @ldionne?

Something is up with std::ranges::begin with this new constructor. Consider the following snippet:

template<class T>
concept can_call_ranges_begin = requires(T& t) {
  std::ranges::begin(t);
};

static_assert(can_call_ranges_begin<fs::path>);
static_assert(can_call_ranges_begin<const fs::path>);

The two static_asserts pass on top of tree as you'd expect (since both path and const path satisfy std::ranges::range concept after all). However, with this new string_view constructor, the second static_assert fails. That is, none of the three operator() in the ranges::begin definition are satisfied, so we hard error and fall through to the deleted operator(). I don't yet understand why this changes with the new string_view constructor though. Any hints, @Quuxplusone?

[• Quuxplusone]

@jloser: Found it! You need to do this in <filesystem>:

   friend _LIBCPP_INLINE_VISIBILITY bool operator==(const path& __lhs, const path& __rhs) noexcept {
-    return __lhs.compare(__rhs) == 0;
+    return __lhs.__compare(__rhs.__pn_) == 0;
   }

(and likewise for each of path's six comparison operators).
The problem is that instantiating path instantiates operator==, which triggers overload resolution on __lhs.compare(__rhs), which evaluates whether __rhs can be converted to string_view... and at that point, it cannot (because its iterator type is still incomplete at that point). So, it goes boom. The hack-around is to make sure we don't call anything that might ever try to convert path to string_view before its time.

[• Quuxplusone]

Here's a reduced test case, as I understand the issue to be: https://godbolt.org/z/qcaf76EbW
Uncomment internalDetail(StringView) to see things blow up on Clang and GCC.
Expect a blog post about this, soon. ;)

[jloser]

Fix path operators to avoid hard error - thanks Arthur!

Still need to test locally whether llvm builds fine with this libc++ change - see the aforementioned bug.

[jloser]

In D113161#3145181, @Quuxplusone wrote:

@jloser: Found it! You need to do this in <filesystem>:

   friend _LIBCPP_INLINE_VISIBILITY bool operator==(const path& __lhs, const path& __rhs) noexcept {
-    return __lhs.compare(__rhs) == 0;
+    return __lhs.__compare(__rhs.__pn_) == 0;
   }

(and likewise for each of path's six comparison operators).
The problem is that instantiating path instantiates operator==, which triggers overload resolution on __lhs.compare(__rhs), which evaluates whether __rhs can be converted to string_view... and at that point, it cannot (because its iterator type is still incomplete at that point). So, it goes boom. The hack-around is to make sure we don't call anything that might ever try to convert path to string_view before its time.

Ah! That makes total sense. I think the thing that threw me off is the constness of the problem. It works fine for path but not const path because of the const-qualified-ness of the compare functions, right?

I just pushed a fix with these path operator changes - let's see what CI thinks. I still need to test locally with llvm + this patched libc++ to see whether we have compile errors in the mentioned bug.

[jloser]

In D113161#3145182, @Quuxplusone wrote:

Here's a reduced test case, as I understand the issue to be: https://godbolt.org/z/qcaf76EbW
Uncomment internalDetail(StringView) to see things blow up on Clang and GCC.
Expect a blog post about this, soon. ;)

Nice reduced test case! I think this is *close* but not exact, right? In your Godbolt,

static_assert(std::ranges::range<const Path>);

passes, but

static_assert(std::ranges::range<Path>);

produces the hard error. This is the inverse of the problem we hit here in libc++. In any case, I look forward to the blog post - happy to provide you fun material in exchange for the problem solved ;)

[jloser]

Always call public compare still in path operators. I think this is fine since we explicitly call the string_type compare overload.

[mizvekov]

By the way, the issues you see with concepts here are related to the cache, and how invalidation is not implemented.

There is a frontend (-cc1) option to disable it, -fno-concept-satisfaction-caching, but the driver part seems to have never been wired up.

[jloser]

Fix from_range.pass.cpp for no wide char support

[jloser]

Try -fno-delayed-template-parsing to make clang-cl happy maybe

[jloser]

This comment has been deleted.

libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp

12 ↗

(On Diff #389211)

@ldionne @Mordante @Quuxplusone I think this will be needed to make clang-cl happy with this test. However, GCC doesn't understand this this compiler flag:

g++-11: error: unrecognized command-line option '-fno-delayed-template-parsing'

What's the syntax to make this additional compile flag only apply to clang-cl case?

[jloser]

Use compare to avoid overload resolution
Remove -fno-delayed-template-parsing for now. It may not be needed at all if we always call compare and avoid overload resolution.

[jloser]

XFAIL msvc for range_concept_conformance.compile.pass.cpp

[jloser]

Rebase and fix summary

[jloser]

Try to XFAIL MinGW for this test. Add explanation of atomic constraints caching.

[• Quuxplusone]

libcxx/include/filesystem

1510

(FWIW, I think you were right that you could use __lhs.compare(__rhs.__pn_) instead of my suggested __lhs.__compare(__rhs.__pn_); I just prefer __compare because it's one less layer of indirection.)

libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp

13–16 ↗

(On Diff #389337)

This sounds sketchy to me. Can you or @mizvekov explain further? (Explain in a thread here, that is. The code comment can be amended, if needed, after I get it :))

This test just includes a couple of libc++ headers and then tests some properties of fs::path. If this simple stuff doesn't work, then that smells like a libc++ bug (still), not something for the end-user to work around. In particular, this smells like we still have the same kind of issue as in __lhs.compare(__rhs), but with some other member function, and in a codepath that gets compiled only on Windows... no?

[mizvekov]

libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp

13–16 ↗

(On Diff #389337)

I don't think caching is any different between clang and clang-cl.

I think the initial issue, from what I read about it, has to do with concepts satisfaction caching and how clang does not implement any kind of invalidation of the cache.

This could be tested by passing frontend command line switch -fno-concept-satisfaction-caching, or if using the driver, -Xclang -fno-concept-satisfaction-caching.

But the difference in behavior between clang and clang-cl must be something else.

[jloser]

libcxx/include/filesystem

1510

Yeah, we can call the public compare member function. But calling __compare avoids overload resolution (see, I read your blog post!), so we can do that to be slightly cheaper.

libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp

13–16 ↗

(On Diff #389337)

I'll precursor my comment with the fact that I don't have a great way to test the clang-cl or MinGW failing builds locally. It was failing for similar reasons with atomic constraints caching on CI. I think it's a similar issue with the __lhs.compare(__rhs) as you mentioned that is only a codepath for when _LIBCPP_WIN32API is defined.

I've reproduced the issue locally with ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_WIN32API=1 to emulate a "windows only code path" that this error shows up. I'll try debugging it soon to figure out the problem.

[jloser]

Poke CI with possible Windows fix for path

[jloser]

@ldionne why does the clang-cl dynamic build fail but clang-cl static build succeed? The dynamic build hard errors about use of [[no_unique_address]] unrelated to this change I'd argue (it exists on top-of-tree). Relevant logs are at https://buildkite.com/llvm-project/libcxx-ci/builds/6840.

Note the warnings show up in both clang-cl dynamic and clang-cl static but only hard errors in the dynamic case.

[mstorsjo]

In D113161#3150454, @jloser wrote:

@ldionne why does the clang-cl dynamic build fail but clang-cl static build succeed? The dynamic build hard errors about use of [[no_unique_address]] unrelated to this change I'd argue (it exists on top-of-tree). Relevant logs are at https://buildkite.com/llvm-project/libcxx-ci/builds/6840.

Note the warnings show up in both clang-cl dynamic and clang-cl static but only hard errors in the dynamic case.

The error isn't about [[no_unique_address]], the actual error is further up in the log:

FAILED: libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.obj 
"C:\Program Files\LLVM\bin\clang-cl.exe"  /nologo -TP -DNDEBUG -DUNICODE -D_ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH -D_ALLOW_MSC_VER_MISMATCH -D_CRTBLD -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\ws\w5\llvm-project\libcxx-ci\libcxx\src -D_LIBCPP_HAS_NO_INT128 /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /Brepro /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw --target=x86_64-pc-windows-msvc /MD /Zi /O2 /Ob1 /DNDEBUG --target=x86_64-pc-windows-msvc -W4 -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-c++11-compat -Wno-undef -Wno-reserved-id-macro -Wno-gnu-include-next -Wno-gcc-compat -Wno-zero-as-null-pointer-constant -Wno-deprecated-dynamic-exception-spec -Wno-sign-conversion -Wno-old-style-cast -Wno-deprecated -Wno-shift-sign-overflow -Wno-double-promotion -Wno-error -EHsc /IC:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1 -std:c++latest /showIncludes /Folibcxx\src\CMakeFiles\cxx_shared.dir\filesystem\operations.cpp.obj /Fdlibcxx\src\CMakeFiles\cxx_shared.dir\ -c -- C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp
In file included from C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp:9:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\filesystem:252:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\iterator:588:
C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\__iterator/counted_iterator.h(68,5): warning: unknown attribute 'no_unique_address' ignored [-Wunknown-attributes]
  [[no_unique_address]] _Iter __current_ = _Iter();
    ^~~~~~~~~~~~~~~~~
In file included from C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp:9:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\filesystem:255:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string:532:
C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string_view(764,6): error: definition with same mangled name '??$?9_WU?$char_traits@_W@__1@std@@@__1@std@@YA_NV?$basic_string_view@_WU?$char_traits@_W@__1@std@@@01@0@Z' as another definition
bool operator!=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
     ^
C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string_view(773,6): note: previous definition is here
bool operator!=(basic_string_view<_CharT, _Traits> __lhs,
     ^
1 warning and 1 error generated.

(That said, those warnings are super noisy, it'd be great to get rid of them - I've been meaning to take a look at it but haven't gotten to it.)

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp

13–16 ↗

(On Diff #389337)

Also just for future reference, if you want to XFAIL something for windows overall (both clang-cl and mingw), it's enough with just XFAIL: windows, or XFAIL: {{.+}}-windows{{.*}} for regex matching the triple for both variants).

[jloser]

Try to avoid definition of std::string_view::operator!= with same mangled name as another definition on windows builds.

This is done by introducing a backported std::type_identity_t and using it in string_view::operator!=. It's backported since we supported string_view in earlier standards and std::type_identity_t is C++20 or later.

I think this workaround may not be needed once we implement operator<=> for string_view and then we'd remove operator!= and friends in favor of operator== and operator<=>.

[jloser]

In D113161#3150604, @mstorsjo wrote:

In D113161#3150454, @jloser wrote:

@ldionne why does the clang-cl dynamic build fail but clang-cl static build succeed? The dynamic build hard errors about use of [[no_unique_address]] unrelated to this change I'd argue (it exists on top-of-tree). Relevant logs are at https://buildkite.com/llvm-project/libcxx-ci/builds/6840.

Note the warnings show up in both clang-cl dynamic and clang-cl static but only hard errors in the dynamic case.

The error isn't about [[no_unique_address]], the actual error is further up in the log:

FAILED: libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.obj 
"C:\Program Files\LLVM\bin\clang-cl.exe"  /nologo -TP -DNDEBUG -DUNICODE -D_ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH -D_ALLOW_MSC_VER_MISMATCH -D_CRTBLD -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\ws\w5\llvm-project\libcxx-ci\libcxx\src -D_LIBCPP_HAS_NO_INT128 /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /Brepro /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw --target=x86_64-pc-windows-msvc /MD /Zi /O2 /Ob1 /DNDEBUG --target=x86_64-pc-windows-msvc -W4 -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-c++11-compat -Wno-undef -Wno-reserved-id-macro -Wno-gnu-include-next -Wno-gcc-compat -Wno-zero-as-null-pointer-constant -Wno-deprecated-dynamic-exception-spec -Wno-sign-conversion -Wno-old-style-cast -Wno-deprecated -Wno-shift-sign-overflow -Wno-double-promotion -Wno-error -EHsc /IC:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1 -std:c++latest /showIncludes /Folibcxx\src\CMakeFiles\cxx_shared.dir\filesystem\operations.cpp.obj /Fdlibcxx\src\CMakeFiles\cxx_shared.dir\ -c -- C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp
In file included from C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp:9:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\filesystem:252:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\iterator:588:
C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\__iterator/counted_iterator.h(68,5): warning: unknown attribute 'no_unique_address' ignored [-Wunknown-attributes]
  [[no_unique_address]] _Iter __current_ = _Iter();
    ^~~~~~~~~~~~~~~~~
In file included from C:\ws\w5\llvm-project\libcxx-ci\libcxx\src\filesystem\operations.cpp:9:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\filesystem:255:
In file included from C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string:532:
C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string_view(764,6): error: definition with same mangled name '??$?9_WU?$char_traits@_W@__1@std@@@__1@std@@YA_NV?$basic_string_view@_WU?$char_traits@_W@__1@std@@@01@0@Z' as another definition
bool operator!=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
     ^
C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1\string_view(773,6): note: previous definition is here
bool operator!=(basic_string_view<_CharT, _Traits> __lhs,
     ^
1 warning and 1 error generated.

(That said, those warnings are super noisy, it'd be great to get rid of them - I've been meaning to take a look at it but haven't gotten to it.)

Thanks for spotting that, @mstorsjo! It does look indeed related to my fix for the atomic constraints caching issue on windows for path. I've just pushed a workaround for string_view::operator!= which may work - we'll see what CI thinks. :)

Note the comparison ops for string_view already have this "issue" with duplicate mangled names I think - it's just not getting hit from call sites in libc++ in a dynamic library CI case before this change, right?

libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp

13–16 ↗

(On Diff #389337)

Good to know - thanks!

[jloser]

Fix backport of type_identity_t to use type_identity, not type_identity

[jloser]

Fix C++03 mode for string_view::operator!= by not cuddling angles

[Mordante]

Mainly looks good, a few small points.

libcxx/include/string_view

291

I think it would be good to commit this separately from this patch.

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

61

Can you test whether it's !is_constructible, when the not used as const?

128

The paper has a Thows clause Throws:Any exception thrown byranges::data(r)andranges::size(r).
Can you add some tests to verify that behaviour?

[jloser]

libcxx/include/string_view

291

I split this apart into https://reviews.llvm.org/D114561

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

61

I think that's a bit tricky. For example,

static_assert(!std::is_constructible_v<std::string_view, decltype(d)>); // fails oddly enough

but

std::string_view s = d; // hard error
assert(s == "test");

hard errors when trying to construct the std::string_view since it invokes the deleted implicit conversion operator:

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:66:20: error: conversion function from 'DeletedStringViewConversionOperator' to 'std::string_view' (aka 'basic_string_view<char>') invokes a deleted function
  std::string_view s = d;

Did you have something specifically in mind that you think could work?

128

Should be doable - good idea. I'll look into that once I figure out the duplicate symbol issue for windows-dll for std::string_view::operator!=. My attempted fix using __type_identity_t doesn't work.

[• Quuxplusone]

libcxx/include/string_view

761–789

IIUC, the three overloads (new lines 764, 773, 783) are intended to accomplish https://en.cppreference.com/w/cpp/string/basic_string_view/operator_cmp 's wording "The implementation shall provide sufficient additional constexpr and noexcept overloads of these functions so that a basic_string_view<CharT,Traits> object sv may be compared to another object t with an implicit conversion to basic_string_view<CharT,Traits>, with semantics identical to comparing sv and basic_string_view<CharT,Traits>(t)."

(1) I don't see how anything in this PR should have affected these overloads at all. I believe it makes sense to me that MSVC wouldn't like these overloads, because MSVC (non-conformingly) doesn't let the spelling of a type affect its mangling — __type_identity_t<BSV> and BSV are going to get the same mangling, which is going to be bad news for MSVC. But, this was already the case prior to your patch, when the parameters had types common_type<BSV>::type and BSV. MSVC should already have been complaining. If it wasn't, then either I don't understand something, or maybe there was just a hole in our test coverage. (Basically we need a test case that instantiates both sv == "hello" and "hello" == sv in the same TU; my hypothesis is that MSVC will barf at that, even before this patch.)
Evidence in favor: https://godbolt.org/z/1MMaK9P33

(2) If we believe it's worthwhile to depart from the status quo and start messing with these overloads, then I would like to see us just take the leap and start using hidden friends for these operators. Hidden friend idiom solves the problem instantly AFAIK.

(3) We certainly should implement <=> for string_view, too, as long as we're messing with this code; but I hypothesize that <=> will not solve any problem here: <=> will have the same problems as == does today, and == will continue having those problems anyway in modes prior to -std=c++20. So I encourage Someone to tackle that, but with the understanding that it's not going to fix anything. ;)

[• Quuxplusone]

libcxx/include/type_traits

1417–1418 ↗

(On Diff #389566)

Moot now, but FYI, we already have this under the name __identity_t.

[jloser]

libcxx/include/string_view

761–789

That's my understanding as well for the additional comparison op overloads.

(1) It's missing test coverage in libc++ I think. One of my changes in Windows-only codepath in`path` is calling a std::string_view::operator!= overload and *somewhere else* we must also be calling *a different* std::string_view::operator!= overload which causes Windows DLL builds to complain. So, it's an issue on top-of-tree but no test provoking the behavior as I understand it. As for the mangling non-conformingness, that's unfortunate. Thanks for the Godbolt link to show me how I could iterate on the issue a bit quicker without the full CI turnaround times. :)

(2) Perhaps. In order to make progress with this patch, I can either XFAIL the windows-dll build and then fix the comparison operator overload issues. Or fix the comparison operator overloads first on trunk and then rebase this patch with it. I don't feel too strongly, but it seems more polite to fix the the comparison operator overloads first and delay this patch (despite me caring more about this patch than the windows-dll problems :)). WDYT?

(3) Shame that it won't fix this problem, but I think you're right.

libcxx/include/type_traits

1417–1418 ↗

(On Diff #389566)

Good to know, thanks. I didn't look around too hard since it was mostly a stab to try to get windows-dll builds working. I'll remove this anyway soon as you know.

[jloser]

Try to workaround windows-dll failure by explicitly constructing the appropriate string_view before calling the operator!= from path.

[jloser]

libcxx/include/string_view

761–789

@Quuxplusone making the comparison operators hidden friends would make our conforming implementation non-conforming. They are required to be non-member functions according to the standard.

I spoke with @davidstone just a bit ago and we think that a paper could be written to make these hidden friends instead. But right now, it would be non-conforming for libc++ to turn them into hidden friends as far as I understand it.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1614r2.html#was-well-formed-now-ill-formed

[jloser]

libcxx/include/filesystem

1036

@Quuxplusone I'm trying to explicitly create the string_view now here which may just dance around the windows-dll issue since we won't try to instantiate the operator!= overload where the RHS is convertible to string_view - we just stick with operator!=(std::string_view, std::string_view) overload exclusively. But it's still a problem for another patch probably. :)

[• Quuxplusone]

libcxx/include/string_view

761–789

making the comparison operators hidden friends would make our implementation non-conforming. They are required to be non-member functions [...] right now, it would be non-conforming for libc++ to turn them into hidden friends as far as I understand it.

I believe that's all correct; but, IMO it's still worth taking the plunge. It's clearly where the Standard is going to wind up, in 3 or 6 or 9 or 12 years, and part (only part!) of what's holding it back is that implementors aren't providing "existing practice" to standardize. The quote from p1614r2 seems to me like evidence supporting "Just hidden-friend everything already": Barry seems to be saying that LEWG knew that hidden-friending operator<=> would change behavior in this obscure corner case, and they decided that it was totally worth it — p1614r2 was adopted, right? even with this breakage explicitly called out in a dedicated section of the paper.

I think I'm getting nerdsniped into both implementing <=> for string_view and refactoring these other operators. I'll make a PR. :) For purposes of D113161, the situation seems to be "Pre-existing incompatibility with MSVC means that some of @jloser's new tests will fail on msvc (only prior to C++20?)," and the solution is to UNSUPPORTED those new tests with a comment in the commit message. And then maybe one of my later PRs will remove that UNSUPPORTEDness. Does that make sense as a way forward (while keeping this PR simple)?

[jloser]

libcxx/include/string_view

761–789

@Quuxplusone Successful nerd snipe then. :)

I do agree that this is where the Standard should be heading - with string_view and other class templates that are mandated to have the operators as non-member functions. I'm not aware of any papers in flight, but I'd be happy to work in that area if you're interested. Prior implementation experience never hurts. In practice, making these string_view operators hidden friends shouldn't break much code out in the wild I imagine.

As for the UNSUPPORTED for the purposes of this PR, is that possible? The problem is happening when the windows-dll build is building libc++ as I understand it - not when running a particular test. Would you just XFAIL all of my new tests and then it won't even compile them for windows-dll build? I was under the impression it would compile them still but they'd be allowed to fail. Or am I missing something?

[• Quuxplusone]

libcxx/include/string_view

761–789

Ah, if the failure is while building the stuff in src/ then UNSUPPORTED/XFAIL won't be sufficient. I thought the failure was specifically on one of your new tests, like, if you write

(void)(sv == "hello");
(void)("hello" == sv);

in a new test, it'll explode. But that was always the case; such a test would blow up MSVC today, if we had such a test, which we don't.

So I'm still confused about what you're saying has changed in this PR. The "sufficient overloads" are poison to MSVC both before and after this patch; nothing has changed that I can see.

Btw, Microsoft STL works around this by giving the "additional sufficient overloads" additional defaulted template parameters: https://github.com/microsoft/STL/blob/main/stl/inc/xstring#L1854 and I suppose we can work around it the same way.

[jloser]

Add defaulted template parameter to each "sufficient overload" for string_view comparison ops to please msvc.

[jloser]

Add tests for when ranges::size() or ranges::data() throws

[jloser]

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

128

Added some tests for both cases! Clang doesn't like throws in a constexpr member function like size() or data(), so they're just a normal runtime test.

[jloser]

libcxx/include/string_view

761–789

I added the default template parameters to the "sufficient overloads" so msvc will be happy with this PR (don't love it, but it's the simplest change to make likely).

I don't yet fully understand what has changed in this PR to invoke the operator!= problem on msvc. I *think* somewhere in filesystem in a Windows only codepath, we must be invoking two different string_view::operator!= - one with bsv/bsv as its arguments and another with bsv/type convertible to bsv that would invoke the other sufficient overload and cause the mangling issue. I've tried to only invoke the bsv/bsv std::string_view::operator!=, but it seems we haven't caught all the cases. I only intentionally touched the places in path that were causing atomic constraints caching problems; it just happened to be that the one in operator/= for windows happens to cause the mangled symbol issues AFAICT. I think it must be here or in that neighborhood since I split up the path comparison operator changes in https://reviews.llvm.org/D114570 and CI passed fine on that.

I'm not terrible interested in spending a lot of time on it given it's annoying to figure out locally without access to windows. I think it won't matter soon since if I understand correctly, you volunteered (thanks!) to:

• Expand on the sufficient overloads tests if needed when rewriting them to be hidden friends or when working on operator<=> for string_view.

We also already worked around the issue by adding default template arguments.

[Mordante]

I really would like to understand why the test fails before approving.
I might want to have a look at it myself, because I'm quite curious why it doesn't work.

libcxx/include/type_traits

1417–1418 ↗

(On Diff #389566)

Interesting. I had searched for pre c++20 version of type_identity during review, but didn't find an alternative.

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

61

Interesting, this is the test I expected to work...

128

Yeah there's no constexpr exception handing (yet?), see http://eel.is/c++draft/expr.const#5.26

[ldionne]

libcxx/include/string_view

302

I'm surprised this isn't written in the spec. I assume this is necessary to break ambiguities in case you try to construct a string_view from a string_view const&& or a string_view&? If you construct from a string_view const&/string_view&&, the copy/move constructor respectively should be preferred, but not for their & and const&& counterparts. Is my understanding correct?

Also, do we have a test that breaks if you remove that constraint?

306

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

45

IMO, trying to encode what this test is doing in the function name is the worst of both worlds:

1. You can't use plain english to describe what the test really does in detail, and
2. The function name is long, which makes it really hard to read (especially when there are multiple long identifiers bunched together, I always end up having to re-read really slowly to figure out which one is being referred to -- here you have only one).

IMO, this would be better, directly inside test() below:

constexpr bool test() {
  test<char>();
#ifndef TEST_HAS_NO_WIDE_CHARACTERS
  test<wchar_t>();
#endif
  test<char8_t>();
  test<char16_t>();
  test<char32_t>();

  // Test that we're not trying to use the type's conversion operator to string_view in the constructor.
  {
    DeletedStringViewConversionOperator d;
    std::string_view csv = std::as_const(d);
    assert(csv == "test");

    DeletedConstStringViewConversionOperator dc;
    std::string_view sv = dc;
    assert(sv == "test");
  }

  return true;
}

Also, I think this would be slightly clearer, simply because we don't use as_const a lot in the test suite -- but I do find there's something nice to doing it your way (you could reuse the same variable for const and non-const tests if that was relevant):

DeletedStringViewConversionOperator const d;
std::string_view csv = d;
assert(csv == "test");

Finally, I would pin down the types of variables instead of using deduction guides, so I'd use std::string_view<char> sv instead of std::string_view sv -- this makes it obvious that any potential CTAD has nothing to do with what we're trying to test.

Those are just suggestions, but the one about adding a comment instead of using a lengthy function name is something I'd love if you could address.

159

Clang is correct to not let you do this -- I think the comment adds very little information. WDYT?

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

22–25

Can we add a test that exercises the deduction guide from a range that is *not* a std::string?

[• Quuxplusone]

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

45

I'd use std::string_view<char> sv instead of std::string_view sv

(Brain fart there: string_view is the non-template, basic_string_view<char> is the template. :))

61

I'm not sure I'm parsing either of your comments correctly, but yeah I would have expected it to be simple to add these lines to this test:

static_assert(!std::is_constructible_v<std::string_view, DeletedStringViewConversionOperator>);
static_assert(std::is_constructible_v<std::string_view, const DeletedStringViewConversionOperator>);
static_assert(std::is_constructible_v<std::string_view, DeletedConstStringViewConversionOperator>);
static_assert(!std::is_constructible_v<std::string_view, const DeletedConstStringViewConversionOperator>);

@jloser, are you saying that one of those lines gives a hard error? I don't think it should.

Also, naming nit: the words StringView aren't pulling their weight and could be omitted: DeletedConversionOperator, DeletedConstConversionOperator.

159

+1.

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

22–25

Something like this could do the trick:

#include "test_iterators.h"
struct Widget {
    char16_t *data_ = u16"foo";
    contiguous_iterator<const char16_t*> begin() const { return data_; }
    contiguous_iterator<const char16_t*> end() const { return data_ + 3; }
};
basic_string_view bsv = Widget();
ASSERT_SAME_TYPE(decltype(bsv), std::basic_string_view<char16_t>);

43

Is the lack of static_assert(test()); here an accidental oversight? I see a lot of constexpr above that doesn't make sense otherwise.

[jloser]

Fix GCC CI issue in from_range.pass.cpp
Address some review feedback

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

61

Renamed. Both

static_assert(!std::is_constructible_v<std::string_view, DeletedStringViewConversionOperator>);
static_assert(!std::is_constructible_v<std::string_view, const DeletedConstStringViewConversionOperator>);

fail currently. I'll need to dig into it.

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

43

It's not an oversight. No constexpr string yet.

[jloser]

Fix -fnoexceptions build
Add more tests for const/non-const conversion op in from_range.pass.cpp
Add deduction tests with non-string type

[jloser]

libcxx/include/string_view

302

It is written in the Standard as the first constraint:

remove_­cvref_­t<R> is not the same type as basic_­string_­view

A few tests in the string_view suite fail when this constraint is removed. The constraint exists to avoid ambiguities as you stated as I understand it.

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

22–25

@Quuxplusone that LMGTM. I adopted that modulo s/u16/u since we the literals are weird (u8, but u, and U for u16 and u32 literals :)).

[jloser]

Try to fix no exceptions build

[ldionne]

LGTM with some comments, thanks a lot!

libcxx/include/string_view

302

Ahaha I just checked again and I don't know how I could miss that! Thanks for correcting me.

742

Can you add a short comment here saying something like

// The dummy default template parameters are used to work around a MSVC issue with mangling, see <link-to-issue> for details.

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

82–85

Please use scopes to delimit the different test cases:

{
  NonConstConversionOperator nc;
  std::string_view sv1  = nc;
  assert(sv1 == "NonConstConversionOp");
  static_assert(!std::is_constructible_v<std::string_view, const NonConstConversionOperator&>); // conversion operator is non-const
}

This makes it easy to see at a glance that this is a test case of its own. It also allows you to keep simple identifiers without numbers in them.

[jloser]

Address Louis's comments:

• use scopes in testing in from_range.pass.cpp
• mention MSVC bug number in string_view sufficient overloads

If CI is happy, I'll land this later tonight.

libcxx/docs/Status/Cxx2bPapers.csv

 "`P0401R6 <https://wg21.link/P0401R6>`__","LWG","Providing size feedback in the Allocator interface","June 2021","",
 "`P0448R4 <https://wg21.link/P0448R4>`__","LWG","A strstream replacement using span<charT> as buffer","June 2021","",""
 "`P1132R8 <https://wg21.link/P1132R8>`__","LWG","out_ptr - a scalable output pointer abstraction","June 2021","",""
 "`P1328R1 <https://wg21.link/P1328R1>`__","LWG","Making std::type_info::operator== constexpr","June 2021","",""
 "`P1425R4 <https://wg21.link/P1425R4>`__","LWG","Iterators pair constructors for stack and queue","June 2021","",""
 "`P1518R2 <https://wg21.link/P1518R2>`__","LWG","Stop overconstraining allocators in container deduction guides","June 2021","|Complete|","13.0"
 "`P1659R3 <https://wg21.link/P1659R3>`__","LWG","starts_with and ends_with","June 2021","",""
 "`P1951R1 <https://wg21.link/P1951R1>`__","LWG","Default Arguments for pair Forwarding Constructor","June 2021","|Complete|","14.0"
-"`P1989R2 <https://wg21.link/P1989R2>`__","LWG","Range constructor for std::string_view","June 2021","",""
+"`P1989R2 <https://wg21.link/P1989R2>`__","LWG","Range constructor for std::string_view","June 2021","|Complete|","14.0"
 "`P2136R3 <https://wg21.link/P2136R3>`__","LWG","invoke_r","June 2021","",""
 "`P2166R1 <https://wg21.link/P2166R1>`__","LWG","A Proposal to Prohibit std::basic_string and std::basic_string_view construction from nullptr","June 2021","|Complete|","13.0"
 "","","","","",""
 "`P0288R9 <https://wg21.link/P0288R9>`__","LWG","``any_invocable``","October 2021","",""
 "`P0798R8 <https://wg21.link/P0798R8>`__","LWG","Monadic operations for ``std::optional``","October 2021","",""
 "`P0849R8 <https://wg21.link/P0849R8>`__","LWG","``auto(x)``: ``DECAY_COPY`` in the language","October 2021","",""
 "`P1072R10 <https://wg21.link/P1072R10>`__","LWG","``basic_string::resize_and_overwrite``","October 2021","",""
 "`P1147R1 <https://wg21.link/P1147R1>`__","LWG","Printing ``volatile`` Pointers","October 2021","|Complete|","14.0"

libcxx/include/filesystem

 
 public:
   // appends
 #if defined(_LIBCPP_WIN32API)
   path& operator/=(const path& __p) {
     auto __p_root_name = __p.__root_name();
     auto __p_root_name_size = __p_root_name.size();
     if (__p.is_absolute() ||
-        (!__p_root_name.empty() && __p_root_name != root_name())) {
+        (!__p_root_name.empty() && __p_root_name != __string_view(root_name().__pn_))) {
       __pn_ = __p.__pn_;
       return *this;
     }
     if (__p.has_root_directory()) {
       path __root_name_str = root_name();
       __pn_ = __root_name_str.native();
       __pn_ += __string_view(__p.__pn_).substr(__p_root_name_size);
       return *this;
#endif // !_LIBCPP_HAS_NO_LOCALIZATION
   }
   friend _LIBCPP_INLINE_VISIBILITY bool operator<=(const path& __lhs, const path& __rhs) noexcept {
     return __lhs.__compare(__rhs.__pn_) <= 0;
   }
   friend _LIBCPP_INLINE_VISIBILITY bool operator>(const path& __lhs, const path& __rhs) noexcept {
     return __lhs.__compare(__rhs.__pn_) > 0;
   }
   friend _LIBCPP_INLINE_VISIBILITY bool operator>=(const path& __lhs, const path& __rhs) noexcept {
     return __lhs.__compare(__rhs.__pn_) >= 0;
   }
 
   friend _LIBCPP_INLINE_VISIBILITY path operator/(const path& __lhs,
                                                   const path& __rhs) {
     path __result(__lhs);
     __result /= __rhs;
     return __result;
   }

libcxx/include/string_view

class basic_string_view {
       constexpr basic_string_view(const basic_string_view&) noexcept = default;
       basic_string_view& operator=(const basic_string_view&) noexcept = default;
       template<class Allocator>
       constexpr basic_string_view(const charT* str);
       basic_string_view(nullptr_t) = delete; // C++2b
       constexpr basic_string_view(const charT* str, size_type len);
       template <class It, class End>
       constexpr basic_string_view(It begin, End end); // C++20
+      template <class Range>
+      constexpr basic_string_view(Range&& r); // C++23
 
       // 7.4, basic_string_view iterator support
       constexpr const_iterator begin() const noexcept;
       constexpr const_iterator end() const noexcept;
       constexpr const_iterator cbegin() const noexcept;
       constexpr const_iterator cend() const noexcept;
       const_reverse_iterator rbegin() const noexcept;
       const_reverse_iterator rend() const noexcept;
namespace std {
      private:
       const_pointer data_;  // exposition only
       size_type     size_;  // exposition only
     };
 
   // basic_string_view deduction guides
   template<class It, class End>
     basic_string_view(It, End) -> basic_string_view<iter_value_t<It>>; // C++20
+  template<class Range>
+    basic_string_view(Range&&) -> basic_string_view<ranges::range_value_t<Range>>; // C++23
 
   // 7.11, Hash support
   template <class T> struct hash;
   template <> struct hash<string_view>;
   template <> struct hash<u8string_view>; // C++20
   template <> struct hash<u16string_view>;
   template <> struct hash<u32string_view>;
   template <> struct hash<wstring_view>;
 
   constexpr basic_string_view<char>     operator "" sv( const char *str,     size_t len ) noexcept;
   constexpr basic_string_view<wchar_t>  operator "" sv( const wchar_t *str,  size_t len ) noexcept;
   constexpr basic_string_view<char8_t>  operator "" sv( const char8_t *str,  size_t len ) noexcept; // C++20
   constexpr basic_string_view<char16_t> operator "" sv( const char16_t *str, size_t len ) noexcept;
   constexpr basic_string_view<char32_t> operator "" sv( const char32_t *str, size_t len ) noexcept;
 
 }  // namespace std
 
 
 */
 
 #include <__config>
 #include <__debug>
+#include <__ranges/concepts.h>
+#include <__ranges/data.h>
 #include <__ranges/enable_borrowed_range.h>
 #include <__ranges/enable_view.h>
+#include <__ranges/size.h>
 #include <__string>
 #include <algorithm>
 #include <compare>
 #include <iosfwd>
 #include <iterator>
 #include <limits>
 #include <stdexcept>
+#include <type_traits>
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
public:
     {
 #if _LIBCPP_STD_VER > 11
     _LIBCPP_ASSERT(__len == 0 || __s != nullptr, "string_view::string_view(_CharT *, size_t): received nullptr");
 #endif
     }
 
 #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
     template <contiguous_iterator _It, sized_sentinel_for<_It> _End>
       requires (is_same_v<iter_value_t<_It>, _CharT> && !is_convertible_v<_End, size_type>)
     constexpr _LIBCPP_HIDE_FROM_ABI basic_string_view(_It __begin, _End __end)
        : __data(_VSTD::to_address(__begin)), __size(__end - __begin)
     {
       _LIBCPP_ASSERT((__end - __begin) >= 0, "std::string_view::string_view(iterator, sentinel) received invalid range");
     }
 #endif
 
+#if _LIBCPP_STD_VER > 20 && !defined(_LIBCPP_HAS_NO_RANGES)
+    template <class _Range>
+      requires (
+        !is_same_v<remove_cvref_t<_Range>, basic_string_view> &&
+        ranges::contiguous_range<_Range> &&
+        ranges::sized_range<_Range> &&
+        is_same_v<ranges::range_value_t<_Range>, _CharT> &&
+        !is_convertible_v<_Range, const _CharT*> &&
+        (!requires(remove_cvref_t<_Range>& d) {
+          d.operator _VSTD::basic_string_view<_CharT, _Traits>();
+        }) &&
+        (!requires {
+         typename remove_reference_t<_Range>::traits_type;
+        } || is_same_v<typename remove_reference_t<_Range>::traits_type, _Traits>)
+      )
+    constexpr _LIBCPP_HIDE_FROM_ABI
+    basic_string_view(_Range&& __r) : __data(ranges::data(__r)), __size(ranges::size(__r)) {}
+#endif
+
     _LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY
     basic_string_view(const _CharT* __s)
         : __data(__s), __size(_VSTD::__char_traits_length_checked<_Traits>(__s)) {}
 
 #if _LIBCPP_STD_VER > 20
     basic_string_view(nullptr_t) = delete;
 #endif
 

 
 // [string.view.deduct]
 
 #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
 template <contiguous_iterator _It, sized_sentinel_for<_It> _End>
   basic_string_view(_It, _End) -> basic_string_view<iter_value_t<_It>>;
 #endif
 
+
+#if _LIBCPP_STD_VER > 20 && !defined(_LIBCPP_HAS_NO_RANGES)
+template <ranges::contiguous_range _Range>
+  basic_string_view(_Range) -> basic_string_view<ranges::range_value_t<_Range>>;
+#endif
+
 // [string.view.comparison]
 // operator ==
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator==(basic_string_view<_CharT, _Traits> __lhs,
                 basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     if ( __lhs.size() != __rhs.size()) return false;
     return __lhs.compare(__rhs) == 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator==(basic_string_view<_CharT, _Traits> __lhs,
                 typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
 {
     if ( __lhs.size() != __rhs.size()) return false;
     return __lhs.compare(__rhs) == 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator==(typename common_type<basic_string_view<_CharT, _Traits> >::type __lhs,
                 basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     if ( __lhs.size() != __rhs.size()) return false;
     return __lhs.compare(__rhs) == 0;
 }
 
 
 // operator !=
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator!=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     if ( __lhs.size() != __rhs.size())
         return true;
     return __lhs.compare(__rhs) != 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator!=(basic_string_view<_CharT, _Traits> __lhs,
                 typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
 {
     if ( __lhs.size() != __rhs.size())
         return true;
     return __lhs.compare(__rhs) != 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator!=(typename common_type<basic_string_view<_CharT, _Traits> >::type __lhs,
                 basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     if ( __lhs.size() != __rhs.size())
         return true;
     return __lhs.compare(__rhs) != 0;
 }
 
 
 // operator <
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator<(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) < 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator<(basic_string_view<_CharT, _Traits> __lhs,
                 typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) < 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator<(typename common_type<basic_string_view<_CharT, _Traits> >::type __lhs,
                 basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) < 0;
 }
 
 
 // operator >
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator> (basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) > 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator>(basic_string_view<_CharT, _Traits> __lhs,
                 typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) > 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator>(typename common_type<basic_string_view<_CharT, _Traits> >::type __lhs,
                 basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) > 0;
 }
 
 
 // operator <=
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator<=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) <= 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator<=(basic_string_view<_CharT, _Traits> __lhs,
                 typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) <= 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator<=(typename common_type<basic_string_view<_CharT, _Traits> >::type __lhs,
                 basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) <= 0;
 }
 
 
 // operator >=
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator>=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) >= 0;
 }
 
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator>=(basic_string_view<_CharT, _Traits> __lhs,
                 typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) >= 0;
 }
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator>=(typename common_type<basic_string_view<_CharT, _Traits> >::type __lhs,
                 basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     return __lhs.compare(__rhs) >= 0;
 }
 
 

libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
+// <string_view>
+
+//  template <class Range>
+//  constexpr basic_string_view(Range&& range);
+
+#include <string_view>
+#include <array>
+#include <cassert>
+#include <iterator>
+#include <ranges>
+#include <type_traits>
+#include <vector>
+
+#include "constexpr_char_traits.h"
+#include "make_string.h"
+#include "test_iterators.h"
+#include "test_range.h"
+
+template<class CharT>
+constexpr void test() {
+  auto data = MAKE_STRING_VIEW(CharT, "test");
+  std::array<CharT, 4> arr;
+  for(int i = 0; i < 4; ++i) {
+    arr[i] = data[i];
+  }
+  auto sv = std::basic_string_view<CharT>(arr);
+
+  ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<CharT>);
+  assert(sv.size() == arr.size());
+  assert(sv.data() == arr.data());
+}
+
+constexpr bool test() {
+  test<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  test<wchar_t>();
+#endif
+  test<char8_t>();
+  test<char16_t>();
+  test<char32_t>();
+
+  struct DeletedConversionOperator {
+    const char* data_ = "test";
+    constexpr const char* begin() const { return data_; }
+    constexpr const char* end() const { return data_ + 4; }
+    operator std::string_view() = delete;
+  };
+
+  struct DeletedConstConversionOperator {
+    const char* data_ = "test";
+    constexpr const char* begin() const { return data_; }
+    constexpr const char* end() const { return data_ + 4; }
+    operator std::string_view() const = delete;
+  };
+
+  //static_assert(!std::is_constructible_v<std::string_view, DeletedConversionOperator>); // TODO: figure out why this fails
+  static_assert(std::is_constructible_v<std::string_view, const DeletedConversionOperator>);
+  static_assert(std::is_constructible_v<std::string_view, DeletedConstConversionOperator>);
+  //static_assert(!std::is_constructible_v<std::string_view, const DeletedConstConversionOperator>); // TODO: figure out why this fails
+
+  // Test that we're not trying to use the type's conversion operator to string_view in the constructor.
+  const DeletedConversionOperator d;
+  std::basic_string_view<char> csv = d;
+  assert(csv == "test");
+
+  DeletedConstConversionOperator dc;
+  std::basic_string_view<char> sv = dc;
+  assert(sv == "test");
+
+  return true;
+}
+
+static_assert(std::is_constructible_v<std::string_view, std::vector<char>&>);
+static_assert(std::is_constructible_v<std::string_view, const std::vector<char>&>);
+static_assert(std::is_constructible_v<std::string_view, std::vector<char>&&>);
+static_assert(std::is_constructible_v<std::string_view, const std::vector<char>&&>);
+
+using SizedButNotContiguousRange = std::ranges::subrange<random_access_iterator<char*>>;
+static_assert(!std::ranges::contiguous_range<SizedButNotContiguousRange>);
+static_assert(std::ranges::sized_range<SizedButNotContiguousRange>);
+static_assert(!std::is_constructible_v<std::string_view, SizedButNotContiguousRange>);
+
+using ContiguousButNotSizedRange = std::ranges::subrange<contiguous_iterator<char*>, sentinel_wrapper<char*>, std::ranges::subrange_kind::unsized>;
+static_assert(std::ranges::contiguous_range<ContiguousButNotSizedRange>);
+static_assert(!std::ranges::sized_range<ContiguousButNotSizedRange>);
+static_assert(!std::is_constructible_v<std::string_view, ContiguousButNotSizedRange>);
+
+static_assert(!std::is_constructible_v<std::string_view, std::vector<char16_t>>); // different CharT
+
+struct WithStringViewConversionOperator {
+  char* begin() const;
+  char* end() const;
+  operator std::string_view() const { return {}; }
+};
+
+static_assert(std::is_constructible_v<std::string_view, WithStringViewConversionOperator>); // lvalue
+static_assert(std::is_constructible_v<std::string_view, const WithStringViewConversionOperator&>); // const lvalue
+static_assert(std::is_constructible_v<std::string_view, WithStringViewConversionOperator&&>); // rvalue
+
+template <class CharTraits>
+struct WithTraitsType {
+  typename CharTraits::char_type* begin() const;
+  typename CharTraits::char_type* end() const;
+  using traits_type = CharTraits;
+};
+
+using CCT = constexpr_char_traits<char>;
+static_assert(std::is_constructible_v<std::string_view, WithTraitsType<std::char_traits<char>>>);
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+static_assert(std::is_constructible_v<std::wstring_view, WithTraitsType<std::char_traits<wchar_t>>>);
+#endif
+static_assert(std::is_constructible_v<std::basic_string_view<char, CCT>, WithTraitsType<CCT>>);
+static_assert(!std::is_constructible_v<std::string_view, WithTraitsType<CCT>>);  // wrong traits type
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+static_assert(!std::is_constructible_v<std::wstring_view, WithTraitsType<std::char_traits<char>>>);  // wrong traits type
+#endif
+
+void test_throwing() {
+  struct ThrowingData {
+    char* begin() const { return nullptr; }
+    char* end() const { return nullptr; }
+    char* data() const { throw 42; return nullptr; }
+  };
+  try {
+    ThrowingData x;
+    (void) std::string_view(x);
+    assert(false);
+  } catch (int i) {
+    assert(i == 42);
+  }
+
+  struct ThrowingSize {
+    char* begin() const { return nullptr; }
+    char* end() const { return nullptr; }
+    size_t size() const { throw 42; return 0; }
+  };
+  try {
+    ThrowingSize x;
+    (void) std::string_view(x);
+    assert(false);
+  } catch (int i) {
+    assert(i == 42);
+  }
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+  test_throwing();
+
+  return 0;
+}
+

libcxx/test/std/strings/string.view/string.view.deduct/iterator_sentinel.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 
 // <string_view>
 
 //  template <contiguous_iterator _It, sized_sentinel_for<_It> _End>
 //    basic_string_view(_It, _End) -> basic_string_view<iter_value_t<_It>>;
 
 #include <string_view>
 #include <cassert>
 
 #include "make_string.h"
 #include "test_macros.h"
 #include "test_iterators.h"
 
 template<class CharT, class Sentinel>
 constexpr void test() {
   auto val = MAKE_STRING_VIEW(CharT, "test");
   auto sv = std::basic_string_view(val.begin(), Sentinel(val.end()));
   ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<CharT>);
   assert(sv.size() == val.size());
   assert(sv.data() == val.data());
 }
 
 constexpr void test() {
   test<char, char*>();
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
   test<wchar_t, wchar_t*>();
 #endif
   test<char8_t, char8_t*>();
   test<char16_t, char16_t*>();
   test<char32_t, char32_t*>();
   test<char, const char*>();
   test<char, sized_sentinel<const char*>>();
 }
 
 int main(int, char**) {
   test();
 
   return 0;
 }
 

libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // UNSUPPORTED: libcpp-no-concepts
 
 // <string_view>
 
-//  template <contiguous_iterator _It, sized_sentinel_for<_It> _End>
-//    basic_string_view(_It, _End) -> basic_string_view<iter_value_t<_It>>;
+//  template<class Range>
+//    basic_string_view(Range&&) -> basic_string_view<ranges::range_value_t<Range>>; // C++23
 
 #include <string_view>
 #include <cassert>
 
 #include "make_string.h"
 #include "test_macros.h"
-#include "test_iterators.h"
 
-template<class CharT, class Sentinel>
-constexpr void test() {
-  auto val = MAKE_STRING_VIEW(CharT, "test");
-  auto sv = std::basic_string_view(val.begin(), Sentinel(val.end()));
+template<class CharT>
+void test() {
+  auto val = MAKE_STRING(CharT, "test");
+  auto sv = std::basic_string_view(val);
   ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<CharT>);
   assert(sv.size() == val.size());
   assert(sv.data() == val.data());
 }
 
-constexpr void test() {
-  test<char, char*>();
+void test() {
+  test<char>();
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
-  test<wchar_t, wchar_t*>();
+  test<wchar_t>();
 #endif
-  test<char8_t, char8_t*>();
-  test<char16_t, char16_t*>();
-  test<char32_t, char32_t*>();
-  test<char, const char*>();
-  test<char, sized_sentinel<const char*>>();
+  test<char8_t>();
+  test<char16_t>();
+  test<char32_t>();
+  test<char>();
 }
 
 int main(int, char**) {
   test();
 
   return 0;
 }