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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a comment.

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



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


================
Comment at: libcxx/include/compare:250
 
+  friend constexpr bool operator==(partial_ordering, partial_ordering) noexcept = default;
+
----------------
I guess this is missing `_LIBCPP_INLINE_VISIBILITY `


================
Comment at: libcxx/include/compare:368
 
+  friend constexpr bool operator==(weak_ordering, weak_ordering) noexcept = default;
+
----------------
Missing `_LIBCPP_INLINE_VISIBILITY` here? 


================
Comment at: libcxx/include/string_view:760
+#else
+template<class _CharT, class _Traits>
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
----------------
Could you define `operator==` outside of the `#ifdef _LIBCPP_HAS_NO_SPACESHIP_OPERATOR` so you do not have to duplicate the logic


================
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);
----------------
Out of curriosity. Do you have a working example where `result` `would *not* be the right comparison category?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D80891