[libcxx-commits] [PATCH] D114912: [libc++] [P1614] Hidden-friend operator<=> for string_view.

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
Comment at: libcxx/include/__compare/comp_cat.h:27
+
+template<class _Traits, class _Dflt, class = void>
+struct __comp_cat {
----------------
What's `_Dflt` short for?  Ditto below in the other alias templates.


================
Comment at: libcxx/include/__compare/comp_cat.h:33
+template<class _Traits, class _Dflt>
+struct __comp_cat<_Traits, _Dflt, typename __void_t<typename _Traits::comparison_category>::type> {
+    using type = typename _Traits::comparison_category;
----------------
Do we need to use `void_t` idiom if this is C++20-or-later code (can we use `requires`?)?


================
Comment at: libcxx/include/string_view:29
     // 7.9, basic_string_view non-member comparison functions
+    // We make these hidden friends, in lieu of "sufficient additional overloads"
     template<class charT, class traits>
----------------
Should we mention the note here that this is non-conforming? :)


================
Comment at: libcxx/include/string_view:728
 inline constexpr bool ranges::enable_borrowed_range<basic_string_view<_CharT, _Traits> > = true;
 #endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
 
----------------
We can probably drop the comment here given how it's just 6 lines up. Note in the next `#if` block below (`#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES))`, we omit the comment since it's short like this one.


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:13
+
+// template<class charT, class traits>
+//   constexpr auto operator<=>(basic_string_view<charT, traits> lhs, basic_string_view<charT, traits> rhs);
----------------
Nit: `s/charT/CharT` and `s/traits/Traits` to be consistent with `libcxx` code in `string_view`? 


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:36
+template<class SV, class OrderingT>
+TEST_CONSTEXPR_CXX14 bool test()
+{
----------------
Since this test is run in C++20-or-later only, let's `s/TEST_CONSTEXPR_CXX14/constexpr/g`, please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114912