This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Mordante on Aug 8 2022, 11:19 AM.

Details

Summary

Implements part of:

  • P1614R2 The Mothership has Landed

Diff Detail

Event Timeline

Mordante created this revision.Aug 8 2022, 11:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 11:19 AM
Mordante requested review of this revision.Aug 8 2022, 11:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 11:19 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 450889.Aug 8 2022, 11:21 AM

Removes some tabs.

Mordante marked an inline comment as not done.Aug 8 2022, 11:25 AM
Mordante added inline comments.
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.

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>

Please also update the SpaceshipProjects.csv

Mordante marked 5 inline comments as done.Aug 10 2022, 12:11 PM

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 updated this revision to Diff 451599.Aug 10 2022, 12:11 PM
Mordante marked 3 inline comments as done.

Addresses review comments.

avogelsgesang added inline comments.Aug 10 2022, 12:35 PM
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 marked 3 inline comments as done.Aug 10 2022, 12:47 PM
Mordante added inline comments.
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 accepted this revision.Aug 10 2022, 1:32 PM

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 accepted this revision.Aug 10 2022, 2:00 PM
philnik added a subscriber: 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.

This revision is now accepted and ready to land.Aug 10 2022, 2:00 PM
Mordante marked 6 inline comments as done.Aug 14 2022, 4:57 AM

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.

This revision was automatically updated to reflect the committed changes.
Mordante marked 2 inline comments as done.
Mordante marked an inline comment as done.Aug 14 2022, 5:18 AM
Mordante added inline comments.
libcxx/include/string
378

See D131857.