Page MenuHomePhabricator

[libcxx] adds operator<=> for basic_string
Needs ReviewPublic

Authored by cjdb on May 31 2020, 12:04 PM.

Details

Reviewers
EricWF
ldionne
BRevzin
mclow.lists
jfb
mpark
Group Reviewers
Restricted Project
Summary

[libcxx] adds operator<=> for basic_string

  • Adds C++20 operator== for basic_string
  • Adds operator<=> for basic_string
  • Adds tests for operator<=>

Diff Detail

Event Timeline

cjdb created this revision.May 31 2020, 12:04 PM
miscco added a subscriber: miscco.May 31 2020, 11:25 PM
miscco added inline comments.
libcxx/include/string
362

I think we should not delete them from the synopsis as they are used in pre C++20 mode. You could mark them with until C++17 and then spaceship with C++20

3948

I think you are missing this specialization below. That said, I am unsure whether this is actually needed at all.

cjdb updated this revision to Diff 267633.Jun 1 2020, 8:37 AM
cjdb marked 2 inline comments as done.

Applies @miscco's suggestions.

libcxx/include/string
362

Thanks! I was wondering how to address this problem...

3948

I assume Howard had his reasons, so I've moved it outside of the block, just in case. Will delete at someone's request.

cjdb marked an inline comment as not done.Jun 1 2020, 8:38 AM
miscco added inline comments.Jun 1 2020, 10:39 AM
libcxx/include/string
3948

That sounds reasonable. Thanks!

cjdb added reviewers: jfb, mpark.Jun 2 2020, 6:10 PM
curdeius added inline comments.
libcxx/include/string
417

Shouldn't notes like "(Until C++17)" be just trailing comments?

libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp
36

I may be missing something, but it seems that there are no tests for the basic cases like:
"abc" < "acb"
"cba" > "aaa"
All (non-equal) tests fall into the shorter/longer check.

cjdb updated this revision to Diff 268238.Jun 3 2020, 9:29 AM
cjdb marked 3 inline comments as done.
cjdb added inline comments.
libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp
36

Done. I also added these tests to the inequality tests.

cjdb added a reviewer: Restricted Project.Jun 22 2020, 9:15 AM

Ping.

curdeius marked an inline comment as done.Jun 23 2020, 12:45 AM
curdeius added inline comments.
libcxx/include/string
362

You changed the order of parameters and now this overload is the same as the one below.
And BTW, why removing spaces in <...>? :)

365

I think you should mark the overloads removed in C++20 as removed in C++20 or until C++17 instead of just C++17 as the latter is misleading.

418

Return type is not bool -> see below in standard.

422

This overload doesn't seem to be marked noexcept by the standard: http://eel.is/c++draft/string.syn.

422

Where is the return type? Standard marks it as see below.

3970

I think you should just use noexcept instead of _NOEXCEPT as it should be always available in C++20.

4008

This _NOEXCEPT should be kept as is of course.

libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp
14

bool?

36

Great!