I would suggest to create define like the other we usually find, e.g _LIBCPP_STRING_HAS_NO_SPACESHIP_OPERATOR (Note that they are usually negated)

Then these could all be turned into
#ifndef _LIBCPP_STRING_HAS_NO_SPACESHIP_OPERATOR

I guess this is missing _LIBCPP_INLINE_VISIBILITY

Missing _LIBCPP_INLINE_VISIBILITY here?

Could you define operator== outside of the #ifdef _LIBCPP_HAS_NO_SPACESHIP_OPERATOR so you do not have to duplicate the logic

Out of curriosity. Do you have a working example where result `would *not* be the right comparison category?

Ack, I forgot to clean this one up. What's the advantage of _LIBCPP_STRING_HAS_NO_SPACESHIP_OPERATOR over _LIBCPP_HAS_NO_SPACESHIP_OPERATOR?

Does it need that? We've defined it here, so it should already have inline visibility.

For a live example, see lines 64-79 in the test case below.

I guess the reason it was designed this way is for backwards compatibility. Someone who designed their char_traits specialisation 20 years ago shouldn't be penalised for not having this trait.

I thought that is defined as:

#if !defined(__cpp_impl_three_way_comparison)
    #define _LIBCPP_HAS_NO_SPACESHIP_OPERATOR
#endif

Which honestly should be essentially the same

In the worst case we improved the consitency with all other methods.

While I personally am in favor of reusing code I want to note that this carries some nonzero cost of creating a string_view.

This will almost certainly be optimized aways in RELEASE but will be noticeable in DEBUG.

I am kind of curios what the maintainers think about this. I know @BillyONeal avoids these miniscule pessimizations like the plague.

Please use unconditional includes.

Please use _LIBCPP_STD_VER, never __cplusplus.

Also, isn't this check the same as #if !defined(_LIBCPP_HAS_NO_SPACESHP_OPERATOR).

#if blocks should be left justified.

What's the reason for the different preprocessor guards?

Does this work if we don't have support for the spaceship operator?

Aren't we losing the short circuit for strings of different lengths?
That optimization seems pretty important.

Why are we creating a copy of the string view at all?

Isn't the point of this overload to create a string view in the conversion of the argument to the parameter type?

That is, common_type_t<basic_string_view<_Char, _Traits>> == basic_string_view<_Char, _Traits>, no?

Please don't introduce a dependency on concepts here.

Instead, use something like:

template <class _Tp>
using __test_for_comparison_category = typename _Tp::comparison_category;

if constexpr ( _IsValidExpansion<__test_for_comparison_category, _Traits>::value ) {

Makes sense, reverting. Also applied to <=> below.

Done. Why don't we want concepts here? (Note: this is just so I understand for next time).

Nit: please be consistent. Here you use #ifndef _LIBCPP_HAS_NO_SPACESHIP_OPERATOR and in other places you use #if !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR) (e.g. line 469).

Nit: why using _LIBCPP_CONSTEXPR_AFTER_CXX11 and not just constexpr?

As above, why _LIBCPP_CONSTEXPR_AFTER_CXX11 and not constexpr?

Should it be in the synopsis? Or rather, I don't think it should (according to http://eel.is/c++draft/string.view.synop), but why is this overload needed? Isn't operator<=>(basic_string_view, basic_string_view) enough?

Oh, I got my answer, http://eel.is/c++draft/string.view#comparison.

Isn't it possible to factor out a test-all function from main, mark it constexpr and call it from both run- and compile-time context?

E.g.

template <...>
TEST_CONSTEXPR bool test(...) // as above but with constexpr and return bool.

TEST_CONSTEXPR bool test_all() // the first part of current main and return bool

main() {
  test_all();
  static_assert(test_all(), "");
}

Return type is not bool.

Just noexcept?

Is there a reason strong_ordering should be available when operator<=> isn't? The two are very tightly coupled.

Also, I believe @EricWF suggested I use it: https://reviews.llvm.org/D80891?id=267623#inline-749046.

(Pass-by-const-value is usually a typo for pass-by-const-reference; in this case we want pass-by-value alone.)

If it were a proper declaration, I'd agree. It's a definition, and const-qualifying the parameter is desirable for read-only objects.

... but cppreference seems pretty sure that operator<=>(basic_string_view, basic_string_view) is the correct signature.

I would make this a hidden friend, unless there's a demonstrable reason not to. Saves typing and saves overload-resolution time.

cpprefernece is not the standard. Please do the required readings before submitting your review.

Right now, _LIBCPP_HAS_NO_SPACESHIP_OPERATOR is defined if-and-only-if the compiler defines __cpp_impl_three_way_comparison. It is not (yet) directly related to whether libc++ itself is planning to define __cpp_lib_three_way_comparison — but that might just be because libc++ itself hasn't done anything with __cpp_lib_three_way_comparison yet. This D80891 is one of the first steps toward implementing __cpp_lib_three_way_comparison.

So, okay, if we want to say that libc++ will implement __cpp_lib_three_way_comparison only on compilers that implement __cpp_impl_three_way_comparison (which does seem reasonable), then it makes sense to guard everything <=>-related under __cpp_impl_three_way_comparison (which is to say, under _LIBCPP_HAS_NO_SPACESHIP_OPERATOR).

This logic applies even to
https://en.cppreference.com/w/cpp/utility/compare/compare_strong_order_fallback
By this logic, libc++ should provide std::compare_strong_order_fallback(x, y) for types lacking an operator<=> function, but should not provide std::compare_strong_order_fallback(x, y) on compilers lacking support for the <=> token.

In that case, please just update this line to

#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)

(and ditto throughout).

I see you're working on hidden-friend-ifying some other operators in D101707, so I hope you get around to these ones soon. As you say over there, hidden friends have well-documented advantages for overload resolution.

The standard actually provides a reference implementation for this operator<=>! So I guess I'd recommend copying it: http://eel.is/c++draft/string.view.comparison#example-1
The reference implementation uses type_identity_t<X> as I suggested; indeed common_type_t is not supposed to be involved here.

FWIW I think a lot of the #if _LIBCPP_STD_VER > 17 && ... will become just #if _LIBCPP_STD_VER > 17 in a little over a year, given our new support model.

Right now, _LIBCPP_HAS_NO_SPACESHIP_OPERATOR is defined if-and-only-if the compiler defines __cpp_impl_three_way_comparison. It is not (yet) directly related to whether libc++ itself is planning to define __cpp_lib_three_way_comparison — but that might just be because libc++ itself hasn't done anything with __cpp_lib_three_way_comparison yet. This D80891 is one of the first steps toward implementing __cpp_lib_three_way_comparison.

So, okay, if we want to say that libc++ will implement __cpp_lib_three_way_comparison only on compilers that implement __cpp_impl_three_way_comparison (which does seem reasonable), then it makes sense to guard everything <=>-related under __cpp_impl_three_way_comparison (which is to say, under _LIBCPP_HAS_NO_SPACESHIP_OPERATOR).

This logic applies even to
https://en.cppreference.com/w/cpp/utility/compare/compare_strong_order_fallback
By this logic, libc++ should provide std::compare_strong_order_fallback(x, y) for types lacking an operator<=> function, but should not provide std::compare_strong_order_fallback(x, y) on compilers lacking support for the <=> token.

Looks like we're on the same page here.

(Off-topic: this context is one where I'm perfectly fine with cppreference being used for convenience, because we're not discussing technical details of proposed code.)

In that case, please just update this line to

#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)

(and ditto throughout).

That line's the same as what's present (confirmed by browser find); is there a typo?

I see you're working on hidden-friend-ifying some other operators in D101707, so I hope you get around to these ones soon. As you say over there, hidden friends have well-documented advantages for overload resolution.

I really wish we could do this, but string.view.synop says they're free functions :(
I'll probably put forward some of the other mothership contributions once ranges work becomes embarrassingly parallel, so feel free to call these out if the synopses are more friendly elsewhere.

The standard actually provides a reference implementation for this operator<=>! So I guess I'd recommend copying it: http://eel.is/c++draft/string.view.comparison#example-1
The reference implementation uses type_identity_t<X> as I suggested; indeed common_type_t is not supposed to be involved here.

Thanks, I missed that. On further inspection, it looks like C++17 doesn't use common_type_t either (it uses an alias for decay_t called __identity), so you're right on all counts, not just operator<=>! I'm happy to replace all common_type_ts with an __identity that's switched on C++17/C++20. WDYT?

Having said that, it baffles me that f(T, type_identity_t<T>) isn't another spelling of f(T, T).