[libcxx-commits] [PATCH] D130295: [libc++] Uses operator<=> in string_view

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
Comment at: libcxx/include/string_view:796
+operator<=>(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) noexcept {
+    if constexpr (requires { typename _Traits::comparison_category; })
+        return static_cast<typename _Traits::comparison_category >(__lhs.compare(__rhs) <=> 0);
----------------
We currently repeat this pattern of code three times, which is about when I'd start to look to refactor it into a common detail function -- what do you think?

This will eventually (soon) be needed for `operator<=>` for `std::string` I believe, right?

Either way, from a QoI perspective, what do you think about checking the type of `comparison_category` to make sure it is one of `partial_ordering`, `weak_ordering`, or `strong_ordering` and not some bogus type, like `void` for example?  This would be a bug from the user's perspective and likely IFNDR from the standards perspective, but it could be an easy QoI improvement I think.


================
Comment at: libcxx/include/string_view:802
+
+template <class _CharT, class _Traits, int = 1>
+_LIBCPP_HIDE_FROM_ABI constexpr auto operator<=>(
----------------
We may want to copy the comment from above to indicate the `int = 1` and `int = 2` dummy non-type template parameters are to work around an MSVC mangling issue as noted above in the `operator==`. It says it applies to the other sufficient overloads below for comparison ops, but I wouldn't be opposed to a comment here for spaceship too. Thoughts?


================
Comment at: libcxx/include/string_view:805
+    basic_string_view<_CharT, _Traits> __lhs,
+    typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) noexcept {
+    if constexpr (requires { typename _Traits::comparison_category; })
----------------
Nit: can use `common_type_t` instead - here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130295