[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]

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

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

[• 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

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]

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

[jloser]

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]

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

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]

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?

[• Quuxplusone]

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]

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

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.

[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

24

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

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]

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

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]

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

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.