This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds consistent comparison for `basic_string_view`
AbandonedPublic

Authored by mumbleskates on May 31 2020, 11:02 AM.

Details

Reviewers
EricWF
ldionne
BRevzin
mclow.lists
miscco
Quuxplusone
jdoerfert
cjdb
Group Reviewers
Restricted Project
Summary
  • Adds comparison_category alias to specialised char_traits.
  • Removes operator!= and inequality operators for basic_string_view (in C++20 mode).
  • Adds operator<=> for basic_string_view (in C++20 mode).

Implements parts of P1614 'The Mothership Has Landed'.

Depends on D101708.

Diff Detail

Event Timeline

cjdb created this revision.May 31 2020, 11:02 AM
miscco added a subscriber: miscco.May 31 2020, 11:17 PM

Thanks for working on this. I have some small nits.

libcxx/include/__string
62

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

libcxx/include/compare
250

I guess this is missing _LIBCPP_INLINE_VISIBILITY

368

Missing _LIBCPP_INLINE_VISIBILITY here?

libcxx/include/string_view
782

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

805

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

cjdb updated this revision to Diff 267623.Jun 1 2020, 8:05 AM
cjdb marked 6 inline comments as done.

Applies @miscco's suggestions.

cjdb added a comment.Jun 1 2020, 8:05 AM

Thanks @miscco!

libcxx/include/__string
62

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?

libcxx/include/compare
250

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

libcxx/include/string_view
805

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.

miscco added inline comments.Jun 1 2020, 10:39 AM
libcxx/include/__string
62

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

libcxx/include/compare
250

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

libcxx/include/string_view
615

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.

EricWF added inline comments.Jun 9 2020, 11:03 AM
libcxx/include/__string
63

Please use unconditional includes.

360

Please use _LIBCPP_STD_VER, never __cplusplus.

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

468

#if blocks should be left justified.

603

What's the reason for the different preprocessor guards?

libcxx/include/compare
368

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

libcxx/include/string_view
616

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

cjdb updated this revision to Diff 269654.Jun 9 2020, 1:37 PM
cjdb marked 5 inline comments as done.

Fixes some of @EricWF's comments.

libcxx/include/__string
603

I think I didn't know about _LIBCPP_HAS_NO_SPACESHP_OPERATOR when I was writing these and forgot to change them all.

libcxx/include/compare
368

Can weak_ordering, etc., even be available if we don't have spaceship support?

libcxx/include/string_view
616

It is still present in operator==(basic_string_view, basic_string_view), which is where the body of work happens. Do you want it put back into this "delegating" operator?

Alternatively, I can follow @miscco's note above. I left that unresolved as I felt it was pending further input.

EricWF added inline comments.Jun 9 2020, 3:04 PM
libcxx/include/string_view
615

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?

625

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 ) {
cjdb updated this revision to Diff 269710.Jun 9 2020, 5:16 PM
cjdb marked 7 inline comments as done.

Applies @EricWF's comments.

cjdb added inline comments.Jun 9 2020, 5:18 PM
libcxx/include/string_view
615

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

625

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

cjdb updated this revision to Diff 271737.Jun 18 2020, 9:01 AM
cjdb edited the summary of this revision. (Show Details)

Rebases on D81823.

cjdb updated this revision to Diff 271740.Jun 18 2020, 9:07 AM
cjdb marked an inline comment as done.
cjdb marked 2 inline comments as done.

I think all the comments requesting changes have been addressed now. @miscco @EricWF could you please confirm?

libcxx/include/__string
468

Whoops, I misunderstood this to mean "please justify why you're using an #if block" and not "please remove the spaces before #if". Fixed up now.

cjdb added reviewers: miscco, Restricted Project.Jun 18 2020, 11:21 AM
curdeius marked an inline comment as done.Jun 23 2020, 12:12 AM
curdeius added a subscriber: curdeius.
curdeius added inline comments.
libcxx/include/__string
362

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

libcxx/include/string_view
623

Nit: why using _LIBCPP_CONSTEXPR_AFTER_CXX11 and not just constexpr?

637

As above, why _LIBCPP_CONSTEXPR_AFTER_CXX11 and not constexpr?

638–639

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.

libcxx/test/std/strings/string.view/string.view.comparison/opcmp.string_view.pointer.pass.cpp
66–68

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(), "");
}
curdeius added inline comments.Jun 23 2020, 12:34 AM
libcxx/include/string_view
29

Return type is not bool.

curdeius added inline comments.Jun 23 2020, 12:41 AM
libcxx/include/string_view
639

Just noexcept?

cjdb updated this revision to Diff 342114.Apr 30 2021, 9:35 PM

rebases to HEAD

Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 9:35 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Apr 30 2021, 10:12 PM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/include/__string
842–844

AFAICT, _LIBCPP_HAS_NO_SPACESHIP_OPERATOR controls whether libc++ is able to use the <=> token. It has nothing to do with whether we define the strong_ordering enumeration or anything like that. So this member typedef should be conditioned on #if _LIBCPP_STD_VER > 17 only. Ditto throughout: anywhere you've got something C++20-related under _LIBCPP_HAS_NO_SPACESHIP_OPERATOR, you shouldn't — unless it depends on the compiler being able to parse the <=> token, in which case you should.

libcxx/include/string_view
638

Remove const on lines 703 and 704. (Pass-by-const-value is usually a typo for pass-by-const-reference; in this case we want pass-by-value alone.)
Remove common_type_t on line 704. Or is it doing something? We do have __identity_t<X> now, if you need that... 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.

libcxx/test/std/strings/string.view/string.view.comparison/opcmp.string_view.pointer.pass.cpp
27–30

Instead of passing y by hand, just make it a local variable auto y = (0 <=> x);

52

Add at least one simple test for basic_string_view<wchar_t>, basic_string_view<char8_t>, etc.
Add at least one simple test for basic_string_view<char, CT> where CT has no comparison_category member.
Add at least one simple test for basic_string_view<char, CT> where CT has a non-type member named comparison_category.
Double-check our coverage for situations like

struct T { operator std::string_view() const; };
std::string_view sv;
ASSERT_SAME_TYPE(decltype(sv <=> T()), std::strong_ordering);
ASSERT_SAME_TYPE(decltype(T() <=> sv), std::strong_ordering);
ASSERT_SAME_TYPE(decltype(sv == T()), bool);
ASSERT_SAME_TYPE(decltype(T() == sv), bool);
ASSERT_SAME_TYPE(decltype(sv != T()), bool);
ASSERT_SAME_TYPE(decltype(T() != sv), bool);
66–68

Please match @curdeius's style here: first test_all(); (no assert), then static_assert(test_all()); afterward. This is how it's done in all libc++ tests so far.
Also, obviously, remove [[nodiscard]] from your test function.

This revision now requires changes to proceed.Apr 30 2021, 10:12 PM
cjdb updated this revision to Diff 342210.May 1 2021, 9:30 PM
cjdb marked 6 inline comments as done.
cjdb retitled this revision from [libcxx] adds operator<=> for basic_string_view to [libcxx] adds consistent comparison for `basic_string_view`.
cjdb edited the summary of this revision. (Show Details)
  • adds tests for wstring_view, u8string_view, u16string_view, u32string_view
  • adds tests for char_traits without comparison_category member alias
  • adds TEST_SAME_AS checks for <=> return types
  • adds dependency on D101708
cjdb added inline comments.May 1 2021, 9:30 PM
libcxx/include/__string
842–844

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.

libcxx/include/string_view
638

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

Quuxplusone added inline comments.May 2 2021, 8:29 AM
libcxx/include/__string
842–844

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

libcxx/include/string_view
638

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.

cjdb added inline comments.May 2 2021, 10:25 AM
libcxx/include/__string
842–844

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?

libcxx/include/string_view
638

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

cjdb updated this revision to Diff 344117.May 10 2021, 10:39 AM

rebases to activate CI

cjdb updated this revision to Diff 344296.May 10 2021, 11:43 PM

rebases to activate CI

cjdb updated this revision to Diff 345050.May 12 2021, 10:28 PM

rebases to activate CI

cjdb updated this revision to Diff 345162.May 13 2021, 8:59 AM

rebases to fix CI (and revert local experiment that made its way on to Phab)

mumbleskates added inline comments.
libcxx/include/string_view
26–27

As I understand it, the functions for the operators {<,<=,>=,>,!=} are no longer supposed to be defined in C++20, and should always be synthesized from <=> and == by the compiler. That is, if we are defining operator<=> we should be doing it instead of the five non-== operators. Is there another reason these symbols still need to be defined?

cjdb marked an inline comment as done.Aug 4 2021, 8:18 AM
cjdb added inline comments.
libcxx/include/string_view
26–27

We still need to support pre-C++20 modes, which don't have a spaceship operator. Everything is conditionally guarded to make sure the right mode exposes the correct operators.

This is just a huge comment that exposes what we've implemented: see below for the actual implementation.

cjdb abandoned this revision.Jan 18 2022, 11:53 AM
cjdb marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 1:32 AM
mumbleskates commandeered this revision.Aug 6 2022, 2:30 PM
mumbleskates added a reviewer: cjdb.

Feeling like getting something done this weekend

Feeling like getting something done this weekend

Welcome back @mumbleskates. Unfortunately this review is no longer needed. I implemented this in D130295 which landed earlier this week. I've also been working on std::string. The status page is up-to date https://libcxx.llvm.org/Status/Spaceship.html. Did I miss the reviews for the items on your name; nullopt and monostate?