[libcxx-commits] [PATCH] D80891: [libcxx] adds operator<=> for basic_string_view

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__string:793-795
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
+    typedef strong_ordering comparison_category;
+#endif
----------------
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.


================
Comment at: libcxx/include/string_view:704
+auto operator<=>(const basic_string_view<_CharT, _Traits> __lhs,
+                 const common_type_t<basic_string_view<_CharT, _Traits>> __rhs) noexcept {
+  const auto __result = __lhs.compare(__rhs) <=> 0;
----------------
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.


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/opcmp.string_view.pointer.pass.cpp:26-29
+constexpr void test(const typename S::value_type* lhs, const S& rhs, std::strong_ordering x, std::strong_ordering y) {
+  assert((lhs <=> rhs) == x);
+  assert((rhs <=> lhs) == y);
+}
----------------
Instead of passing `y` by hand, just make it a local variable `auto y = (0 <=> x);`


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/opcmp.string_view.pointer.pass.cpp:51
+  return true;
+}
+
----------------
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);
```


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/opcmp.string_view.pointer.pass.cpp:65-67
+    typedef std::basic_string_view<char, constexpr_char_traits<char> > SV;
+    constexpr SV sv1;
+    constexpr SV sv2{"abcde", 5};
----------------
curdeius wrote:
> 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(), "");
> }
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80891/new/

https://reviews.llvm.org/D80891