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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/string_view:615
 {
-    if ( __lhs.size() != __rhs.size()) return false;
-    return __lhs.compare(__rhs) == 0;
+    return __lhs == basic_string_view<_CharT, _Traits>(__rhs);
 }
----------------
EricWF wrote:
> Why are we creating a copy of the string view at all?
> 
> Isn't the point of this overload to create a string view in the conversion of the argument to the parameter type?
> 
> That is, `common_type_t<basic_string_view<_Char, _Traits>> == basic_string_view<_Char, _Traits>`, no?
Makes sense, reverting. Also applied to <=> below.


================
Comment at: libcxx/include/string_view:625
+    const auto result = __lhs.compare(__rhs) <=> 0;
+    if constexpr (requires { typename _Traits::comparison_category; }) {
+        return static_cast<typename _Traits::comparison_category>(result);
----------------
EricWF wrote:
> Please don't introduce a dependency on concepts here.
> 
> Instead, use something like:
> 
> ```
> template <class _Tp>
> using __test_for_comparison_category = typename _Tp::comparison_category;
> 
> if constexpr ( _IsValidExpansion<__test_for_comparison_category, _Traits>::value ) {
> ```
Done. Why don't we want concepts here? (Note: this is just so I understand for next time).


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

https://reviews.llvm.org/D80891