[libc++] Uses operator<=> in string.

[Mordante]

Summary

Implements part of:

• P1614R2 The Mothership has Landed

[Mordante]

Removes some tabs.

[Mordante]

libcxx/include/string

51

This was implemented in D130295.

4175

This is an optimization that gives a modest performance improvement 210051548ea01357cc834ce8917559ca32b7a137 so decided to keep it. But maybe something to consider during review.

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

64

Note basic_string basic_string_view comparison is part of the string_view header and tested in the string_view tests.

[avogelsgesang]

Thanks for working on this!

libcxx/include/string

49–55

also update the rest of the synopsis to using instead of typedef

50

indentation seems off

4194

_LIBCPP_STD_VER <= 17? would be more consistent with the _LIBCPP_STD_VER > 17 above...

4226

I would prefer to switch this #if around, such that the order of implementation matches the order in the synopsis. Not sure what the previous art/current guidelines are around this, though

4237

afaict, the standard does not declare this as noexcept

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

41

missing tests for the "mixed" comparison operators

AssertOrderAreNotNoexcept<T, CharT>
AssertOrderReturn<Ordering, T, CharT>

[avogelsgesang]

Please also update the SpaceshipProjects.csv

[Mordante]

Thanks for the review!

libcxx/include/string

50

yeah I noticed it to, already fixed.

4226

We don't have a real art/guidelines here. Personally I prefer to use the #if _LIBCPP_STD_VER > XX style.

4237

You're right, good catch.

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

41

I assume you mean const CharT*.

41

We usually don't test AssertOrderAreNotNoexcept; Standard libraries are allowed to mark functions noexcept when the Standard doesn't mandate it. MSVC STL uses this a lot, by adding that validation we would break this test for them. They are actively using our test to validate their implementation.

(Note that Standard libraries aren't allowed to mark functions constexpr when the Standard doesn't mandate it.)

[Mordante]

Addresses review comments.

[avogelsgesang]

libcxx/include/string

4166

we have that basic_string_view already available under the name __self_view

Here and other places.

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

41

I assume you mean const CharT*.

right

We usually don't test AssertOrderAreNotNoexcept; Standard libraries are allowed to mark functions noexcept when the Standard doesn't mandate it. [...]

Thanks for that clarification!

Afaict, AssertOrderReturn<Ordering, T, CharT>() still makes sense, though.

[Mordante]

libcxx/include/string

4166

I didn't know that. Still I prefer to keep the wording similar to that Standard. That makes it easier to verify we did the right thing. I like to use things like __self_view in places where the Standard specifies a behaviour without code.

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

41

Agreed, since CharT is defined on line 42 so that test has been placed on line 43.

[avogelsgesang]

LGTM. Thanks!

libcxx/include/string

4166

makes sense

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

41

Thanks for the pointer. Somehow, I missed that...

[philnik]

LGTM % nits.

libcxx/include/string

378

These shouldn't both be constexpr since C++20 and removed in C++20.

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

10–12

Why do you explain why the test is marked unsupported C++03 - C++17? AFAIK we do that nowhere else.

79

You don't have to return true here. Just from the main test function.

[Mordante]

Thanks for the review.

libcxx/include/string

378

That's how it's implemented at the moment so I prefer to keep the synopsis as is. But we can remove the _LIBCPP_CONSTEXPR_AFTER_CXX17 for the removed functions, that shouldn't affect the tests.

I'll make a followup for that.

libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp

10–12

We don't do that often, but in this case I think the explanation is useful. Somebody might expect all comparisions for all C++ versions in this test.

(In general I think we could have more comments in libc++ ;-) )

79

Good point, I had only test before and refactored it to have an extra layer... and forgot to remove them here.

[Mordante]

libcxx/include/string

378

See D131857.