Page MenuHomePhabricator

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

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

Details

Reviewers
EricWF
ldionne
BRevzin
mclow.lists
miscco
Group Reviewers
Restricted Project
Summary

[libcxx] adds operator<=> for basic_string_view

Changes:
* Adds `comparison_category` alias to specialised char_traits.
* Adds C++20 operator== for basic_string_view
* Adds operator<=> for basic_string_view
* Adds tests for operator<=>

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 ↗(On Diff #267508)

I guess this is missing _LIBCPP_INLINE_VISIBILITY

368 ↗(On Diff #267508)

Missing _LIBCPP_INLINE_VISIBILITY here?

libcxx/include/string_view
806

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

829

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 ↗(On Diff #267508)

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

libcxx/include/string_view
829

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.
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 ↗(On Diff #267508)

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

libcxx/include/string_view
630

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.

602

What's the reason for the different preprocessor guards?

libcxx/include/compare
368 ↗(On Diff #267623)

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

libcxx/include/string_view
631

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
602

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 ↗(On Diff #267623)

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

libcxx/include/string_view
631

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
630

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?

640

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
630

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

640

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
638

Nit: why using _LIBCPP_CONSTEXPR_AFTER_CXX11 and not just constexpr?

652

As above, why _LIBCPP_CONSTEXPR_AFTER_CXX11 and not constexpr?

653–654

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
65–67

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
42

Return type is not bool.

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

Just noexcept?