This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds consistent comparison for `basic_string`
AbandonedPublic

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

Details

Reviewers
EricWF
ldionne
BRevzin
curdeius
Mordante
zoecarver
Quuxplusone
Group Reviewers
Restricted Project
Summary
  • Removes operator!= and inequality operators for basic_string (in C++20 mode).
  • Adds operator<=> for basic_string (in C++20 mode).

Implements parts of P1614 'The Mothership Has Landed'.

Depends on D80891.

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
370

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

4089

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
370

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

4089

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
4089

That sounds reasonable. Thanks!

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

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
370

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

373

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.

426–430

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

430

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

430

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

4111

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

4149

This _NOEXCEPT should be kept as is of course.

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

bool?

36

Great!

cjdb planned changes to this revision.May 4 2021, 1:37 PM

Needs rebase, etc.

cjdb removed a subscriber: curdeius.
cjdb updated this revision to Diff 344475.May 11 2021, 10:39 AM
cjdb retitled this revision from [libcxx] adds operator<=> for basic_string to [libcxx] adds consistent comparison for `basic_string`.
cjdb edited the summary of this revision. (Show Details)

rebases to activate CI and get on people's radar

Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 10:39 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.May 11 2021, 11:00 AM

@cjdb: I think you should go ahead and land the additions to "libcxx/test/std/strings/basic.string/string.nonmembers/" (other than ".../string_opcmp/", obviously). These look like good additions to the test suite, and should pass both before and after your spaceship-related changes. (It is mildly shocking that the old test suite lacked any tests where one operand was not a prefix of the other.)

libcxx/include/string
4037

Either remove [[nodiscard]] (the easy path), or replace it with _LIBCPP_NODISCARD_EXT and put a libcxx/test/libcxx/ test on it.

libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp
13

This comment suggests to me that we should have a new allocation-focused test that verifies that the comparison does no allocations (i.e. that we don't do it by converting the string_view to string, or anything equally dumb). And it should test all seven comparison operators in all modes. But this is largely orthogonal to this patch and could be done in a separate PR. I'll volunteer myself for it if you don't want it.

Separately, I think we need some tests for comparing wstring and maybe even u8string. As string comparison's relationship to char_traits gets more complicated, our existing lack of wstring tests becomes more glaring. This is also largely orthogonal to this patch, and would affect the existing tests for < > <= >= == !=.

24

I don't understand why you pass strong_ordering::foo but then implicitly convert it to weak_ordering here. Do we expect string::operator<=> to return weak_ordering? Why?
I suggest

template<class S, class O>
constexpr void test(const S& lhs, const S& rhs, O expected)
{
    auto actual = (lhs <=> rhs);
    ASSERT_SAME_TYPE(decltype(actual), decltype(expected));
    assert(actual == expected);
}
59

Bizarre whitespace between basic_string and <. I guess it's for consistency with "string_string_view.pass.cpp" line 61; but I'd lose the whitespace in both cases.

This revision now requires changes to proceed.May 11 2021, 11:00 AM
cjdb marked 6 inline comments as done.May 11 2021, 11:35 AM

@cjdb: I think you should go ahead and land the additions to "libcxx/test/std/strings/basic.string/string.nonmembers/" (other than ".../string_opcmp/", obviously). These look like good additions to the test suite, and should pass both before and after your spaceship-related changes.

SGTM, gimme an hour or so.

(It is mildly shocking that the old test suite lacked any tests where one operand was not a prefix of the other.)

If you think that's shocking, check out our container comparison tests.

libcxx/include/string
4037

It's still unclear to me why _LIBCPP_NODISCARD_EXT is desirable over an attribute users can already disable.

libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp
13

This comment suggests to me that we should have a new allocation-focused test that verifies that the comparison does no allocations (i.e. that we don't do it by converting the string_view to string, or anything equally dumb). And it should test all seven comparison operators in all modes. But this is largely orthogonal to this patch and could be done in a separate PR. I'll volunteer myself for it if you don't want it.

If you're concerned about std::string_view(x) doing an allocation: that should be a part of the string_view test suite, not the string comparison suite.
If you're concerned about an arbitrary allocation happening, then I'm not sure how we'd go about doing that in a feasible manner (why would we do this for comparison, and not string::clear, for example?).
Regardless of what you're wanting here, I'm happy to review when the time comes.

Separately, I think we need some tests for comparing wstring and maybe even u8string. As string comparison's relationship to char_traits gets more complicated, our existing lack of wstring tests becomes more glaring. This is also largely orthogonal to this patch, and would affect the existing tests for < > <= >= == !=.

This is a reasonable request, but since we don't have these tests for the other operators, I wonder if it'd be better to do this in a separate patch as one unit (i.e. after this patch lands, but not long after).

24

See below for a whole set of tests. The proposed change sounds reasonable to me: will fix in an hour or so.

Do these changes require updating the status of papers?

libcxx/include/string
4037

The [[nodiscard]] is an extension, can you the _LIBCPP_NODISCARD_EXT macro and add it to the list extensions.

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

Please address the comment above.

31

Can you turn the test into a templated test and test with all allowed string types?
The support header make_string.h has some macros that will be useful for these tests.

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

Please use bool.

libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp
12

Please add the function being tested.

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

No, operator<=> isn't bool though! It's one of the ordering types, and apparently depends on _Traits (so even changing auto to std::strong_ordering wouldn't be right). I think auto is fine.

cjdb marked 4 inline comments as done.May 11 2021, 7:05 PM
cjdb added inline comments.
libcxx/include/string
4037

This doesn't answer my question. Most attributes don't change the meaning of a program, so it doesn't make sense to me why this macro is useful at all. Unless I'm grepping wrong, we don't have a _LIBCPP_NOEXCEPT_EXT macro for when the standard doesn't put noexcept on things. Why is [[nodiscard]] any different? This is at least my fourth time asking this question without an answer; I'd appreciate one please.

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

@curdeius was asking why it was bool, because it shouldn't be, so I've already addressed it.

31

Not sure I understand what you're asking for, but let's take this up on Discord.

libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp
12

There isn't a function that's being tested. See the comment below.

zoecarver added inline comments.May 12 2021, 10:19 AM
libcxx/include/string
4037

This is C++20 mode, so we can probably assume that all compilers support [[nodiscard]], however, and this is a more general rule of thumb, if we decide that it would be beneficial to put [[nodiscard]] somewhere, we should probably use _LIBCPP_NODISCARD_ATTRIBUTE or _LIBCPP_NOEXCEPT_EXT because it works on more platforms (i.e., [[clang::warn_unused_result]] on earlier versions of clang).

For places where the standard doesn't specifically ask us to use nodiscard, I think for now at least, it still makes sense to use _LIBCPP_NODISCARD_EXT (we can have a discussion at some point about retiring this attribute, if you'd like). This allows users to disable [[nodiscard]] if it's causing problems, or they don't want to use it for some reason. We can argue about the merits of such a configuration, but it's something that we have in libc++, and we shouldn't pull the rug from under people, at least not without discussion first.

In the future you can generally look at the definition of such macros to see why they're useful, etc.

cjdb marked an inline comment as done.May 12 2021, 10:35 AM
cjdb added inline comments.
libcxx/include/string
4037

This is C++20 mode, so we can probably assume that all compilers support [[nodiscard]], however, and this is a more general rule of thumb, if we decide that it would be beneficial to put [[nodiscard]] somewhere, we should probably use _LIBCPP_NODISCARD_ATTRIBUTE or _LIBCPP_NOEXCEPT_EXT because it works on more platforms (i.e., [[clang::warn_unused_result]] on earlier versions of clang).

Aren't we now only supporting as far back as LLVM ${CURRENT_VERSION}-2? That would mean this patch only needs to worry about LLVM 11+, and this attribute has been supported since Clang 3.9.

For places where the standard doesn't specifically ask us to use nodiscard, I think for now at least, it still makes sense to use _LIBCPP_NODISCARD_EXT (we can have a discussion at some point about retiring this attribute, if you'd like). This allows users to disable [[nodiscard]] if it's causing problems, or they don't want to use it for some reason. We can argue about the merits of such a configuration, but it's something that we have in libc++, and we shouldn't pull the rug from under people, at least not without discussion first.

In the future you can generally look at the definition of such macros to see why they're useful, etc.

This still doesn't address my original question. Given -Wno-unused-result turns off [[nodiscard]], and is present on both Clang and GCC, I don't see why we want to have two ways to toggle the same thing.

Mordante added inline comments.May 12 2021, 11:37 AM
libcxx/include/string
4037

This doesn't answer my question.

True when I started my review these comments weren't there yet, so our messages crossed.

The problem isn't the compiler support. The problem is that this function isn't declared as [[nodiscard]] in the Standard. In that case we use _LIBCPP_NOEXCEPT_EXT. See https://libcxx.llvm.org/docs/UsingLibcxx.html#libc-extensions for more information. I hope that clarifies the issue.

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

Good point.

cjdb added inline comments.May 12 2021, 1:45 PM
libcxx/include/string
4037

We seem to be going in circles. You're saying "we need to use _LIBCPP_NOEXCEPT_EXT because it's an extension and the user might want to disable it". I'm saying "they already can, with -Wno-unused-result: why do we need two ways to do this?", to which the collective reply seems to be "we need to use _LIBCPP_NOEXCEPT_EXT because it's an extension and the user might want to disable it".

This document (which I'd read before initially asking why) doesn't address my question either.

cjdb abandoned this revision.Jan 18 2022, 11:53 AM
cjdb marked 10 inline comments as done.