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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: libcxx/include/__string:62
+#if !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
+#include <compare>
+#endif // _LIBCPP_HAS_NO_SPACESHIP_OPERATOR
----------------
Please use unconditional includes.


================
Comment at: libcxx/include/__string:363
     typedef mbstate_t state_type;
+    #if __cplusplus > 201703L && defined(__cpp_impl_three_way_comparison) && __cpp_impl_three_way_comparison >= 201907
+    using comparison_category = strong_ordering;
----------------
Please use `_LIBCPP_STD_VER`, never `__cplusplus`.

Also, isn't this check the same as `#if !defined(_LIBCPP_HAS_NO_SPACESHP_OPERATOR)`.


================
Comment at: libcxx/include/__string:469
     typedef mbstate_t state_type;
+    #if !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
+    using comparison_category = strong_ordering;
----------------
`#if` blocks should be left justified.


================
Comment at: libcxx/include/__string:602
     typedef mbstate_t      state_type;
+    #if __cplusplus > 201703L && defined(__cpp_impl_three_way_comparison) && __cpp_impl_three_way_comparison >= 201907
+    using comparison_category = strong_ordering;
----------------
What's the reason for the different preprocessor guards?


================
Comment at: libcxx/include/compare:368
 
+  _LIBCPP_INLINE_VISIBILITY friend constexpr bool operator==(weak_ordering, weak_ordering) noexcept = default;
+
----------------
Does this work if we don't have support for the spaceship operator?


================
Comment at: libcxx/include/string_view:616
+    return __lhs == basic_string_view<_CharT, _Traits>(__rhs);
 }
 
----------------
Aren't we losing the short circuit for strings of different lengths? 
That optimization seems pretty important.


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

https://reviews.llvm.org/D80891