This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jloser on Nov 3 2021, 6:01 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jloser updated this revision to Diff 385231.Nov 5 2021, 10:00 PM

Simplify wrong traits_type test

Quuxplusone added inline comments.
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 added inline comments.Nov 7 2021, 12:01 PM
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?

philnik added a subscriber: philnik.Nov 7 2021, 4:28 PM
Quuxplusone added inline comments.Nov 8 2021, 8:26 AM
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 updated this revision to Diff 386167.Nov 10 2021, 7:50 AM
jloser marked 7 inline comments as done.

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

Also still need to dig into GCC11 failing tests.

jloser added inline comments.Nov 10 2021, 7:52 AM
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>>>);
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 updated this revision to Diff 386204.Nov 10 2021, 9:27 AM

Use char_type from CharTraits in WithTraitsType

jloser marked 2 inline comments as done.Nov 10 2021, 9:28 AM
jloser added inline comments.
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 updated this revision to Diff 386206.Nov 10 2021, 9:29 AM
jloser marked an inline comment as done.

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

jloser marked an inline comment as done.Nov 10 2021, 9:34 AM
jloser added inline comments.
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 marked an inline comment as done.EditedNov 13 2021, 7:50 PM
jloser added a subscriber: cjdb.

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 added a comment.EditedNov 20 2021, 4:45 PM

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?

@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.

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 updated this revision to Diff 388776.Nov 21 2021, 4:28 PM
jloser edited the summary of this revision. (Show Details)

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: 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 added a comment.EditedNov 21 2021, 4:32 PM

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 updated this revision to Diff 388779.Nov 21 2021, 4:41 PM

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

mizvekov added a subscriber: mizvekov.EditedNov 21 2021, 5:06 PM

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 updated this revision to Diff 388781.Nov 21 2021, 6:02 PM

Fix from_range.pass.cpp for no wide char support

jloser updated this revision to Diff 389211.Nov 23 2021, 7:56 AM

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

jloser added a comment.EditedNov 23 2021, 8:34 AM
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 updated this revision to Diff 389259.Nov 23 2021, 10:47 AM
jloser edited the summary of this revision. (Show Details)

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 updated this revision to Diff 389282.Nov 23 2021, 12:34 PM
jloser edited the summary of this revision. (Show Details)

XFAIL msvc for range_concept_conformance.compile.pass.cpp

jloser updated this revision to Diff 389286.Nov 23 2021, 12:36 PM
jloser edited the summary of this revision. (Show Details)

Rebase and fix summary

jloser edited the summary of this revision. (Show Details)Nov 23 2021, 12:39 PM
jloser updated this revision to Diff 389337.Nov 23 2021, 3:29 PM
jloser edited the summary of this revision. (Show Details)

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

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 added inline comments.Nov 23 2021, 5:31 PM
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 marked an inline comment as done.Nov 23 2021, 5:52 PM
jloser added inline comments.
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 updated this revision to Diff 389365.Nov 23 2021, 6:17 PM
jloser marked an inline comment as done.

Poke CI with possible Windows fix for path

jloser added a comment.EditedNov 23 2021, 8:47 PM

@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.

@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 added inline comments.Nov 23 2021, 11:08 PM
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 updated this revision to Diff 389502.Nov 24 2021, 7:53 AM
jloser marked 2 inline comments as done.

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 marked an inline comment as done.Nov 24 2021, 7:57 AM

@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 updated this revision to Diff 389526.Nov 24 2021, 9:42 AM
jloser marked an inline comment as done.

Fix backport of type_identity_t to use type_identity, not type_identity

jloser updated this revision to Diff 389566.Nov 24 2021, 11:05 AM

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

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 added inline comments.Nov 24 2021, 12:47 PM
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.

jloser marked an inline comment as done.Nov 24 2021, 12:47 PM
Quuxplusone requested changes to this revision.Nov 24 2021, 1:21 PM
Quuxplusone added inline comments.
libcxx/include/string_view
763–791

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. ;)

This revision now requires changes to proceed.Nov 24 2021, 1:21 PM
libcxx/include/type_traits
1417–1418 ↗(On Diff #389566)

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

jloser added inline comments.Nov 24 2021, 2:55 PM
libcxx/include/string_view
763–791

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 updated this revision to Diff 389627.Nov 24 2021, 4:30 PM

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

jloser marked an inline comment as done.Nov 24 2021, 4:33 PM
jloser added a subscriber: davidstone.
jloser added inline comments.
libcxx/include/string_view
763–791

@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 added inline comments.Nov 24 2021, 4:35 PM
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. :)

libcxx/include/string_view
763–791

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 added inline comments.Nov 24 2021, 6:08 PM
libcxx/include/string_view
763–791

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

libcxx/include/string_view
763–791

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 updated this revision to Diff 389644.Nov 24 2021, 7:43 PM

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

jloser updated this revision to Diff 389809.Nov 25 2021, 8:25 AM

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

jloser marked 3 inline comments as done.Nov 25 2021, 8:27 AM
jloser added inline comments.
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 added inline comments.Nov 25 2021, 8:37 AM
libcxx/include/string_view
763–791

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.

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 added inline comments.Nov 25 2021, 10:53 AM
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
24

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

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
24

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 updated this revision to Diff 390168.Nov 27 2021, 10:12 AM
jloser marked 8 inline comments as done.
jloser edited the summary of this revision. (Show Details)

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 marked an inline comment as done.Nov 27 2021, 10:13 AM
jloser updated this revision to Diff 390173.Nov 27 2021, 2:42 PM
jloser marked 2 inline comments as done.
jloser edited the summary of this revision. (Show Details)

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 marked an inline comment as done.Nov 27 2021, 2:42 PM
jloser added inline comments.
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
24

@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 updated this revision to Diff 390181.Nov 27 2021, 6:25 PM

Try to fix no exceptions build

ldionne accepted this revision.Nov 29 2021, 8:53 AM

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–744

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 updated this revision to Diff 391172.Dec 1 2021, 5:26 PM
jloser edited the summary of this revision. (Show Details)

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.

jloser marked 2 inline comments as done.Dec 1 2021, 5:57 PM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 1 2021, 8:18 PM
This revision was automatically updated to reflect the committed changes.