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

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

That sounds reasonable. Thanks!

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

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.

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

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.

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

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

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

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

This _NOEXCEPT should be kept as is of course.

bool?

Great!

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

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

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

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

Please address the comment above.

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.

Please use bool.

Please add the function being tested.

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.

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.

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

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

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

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.

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.

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.

Good point.

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.