[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: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;
----------------
EricWF wrote:
> What's the reason for the different preprocessor guards?
I think I didn't know about `_LIBCPP_HAS_NO_SPACESHP_OPERATOR` when I was writing these and forgot to change them all.


================
Comment at: libcxx/include/compare:368
 
+  _LIBCPP_INLINE_VISIBILITY friend constexpr bool operator==(weak_ordering, weak_ordering) noexcept = default;
+
----------------
EricWF wrote:
> Does this work if we don't have support for the spaceship operator?
Can `weak_ordering`, etc., even be available if we don't have spaceship support?


================
Comment at: libcxx/include/string_view:616
+    return __lhs == basic_string_view<_CharT, _Traits>(__rhs);
 }
 
----------------
EricWF wrote:
> Aren't we losing the short circuit for strings of different lengths? 
> That optimization seems pretty important.
It is still present in `operator==(basic_string_view, basic_string_view)`, which is where the body of work happens. Do you want it put back into this "delegating" operator?

Alternatively, I can follow @miscco's note above. I left that unresolved as I felt it was pending further input.


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

https://reviews.llvm.org/D80891