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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

Thanks @miscco!



================
Comment at: libcxx/include/__string:61
 
+#if __cplusplus > 201703L && defined(__cpp_impl_three_way_comparison) && __cpp_impl_three_way_comparison >= 201907
+#include <compare>
----------------
miscco wrote:
> 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`
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`?


================
Comment at: libcxx/include/compare:250
 
+  friend constexpr bool operator==(partial_ordering, partial_ordering) noexcept = default;
+
----------------
miscco wrote:
> I guess this is missing `_LIBCPP_INLINE_VISIBILITY `
Does it need that? We've defined it here, so it should //already// have inline visibility.


================
Comment at: libcxx/include/string_view:783
+    const auto result = __lhs.compare(__rhs) <=> 0;
+    if constexpr (requires { typename _Traits::comparison_category; }) {
+        return static_cast<typename _Traits::comparison_category>(result);
----------------
miscco wrote:
> Out of curriosity. Do you have a working example where `result` `would *not* be the right comparison category?
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.


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

https://reviews.llvm.org/D80891