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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a subscriber: BillyONeal.
miscco added inline comments.


================
Comment at: libcxx/include/__string:61
 
+#if __cplusplus > 201703L && defined(__cpp_impl_three_way_comparison) && __cpp_impl_three_way_comparison >= 201907
+#include <compare>
----------------
cjdb wrote:
> miscco wrote:
> > I would suggest to create define like the other we usually find, e.g `_LIBCPP_STRING_HAS_NO_SPACESHIP_OPERATOR` (Note that they are usually negated)
> > 
> > Then these could all be turned into 
> > `#ifndef _LIBCPP_STRING_HAS_NO_SPACESHIP_OPERATOR`
> Ack, I forgot to clean this one up. What's the advantage of `_LIBCPP_STRING_HAS_NO_SPACESHIP_OPERATOR` over `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR`?
I thought that is defined as:

```
#if !defined(__cpp_impl_three_way_comparison)
    #define _LIBCPP_HAS_NO_SPACESHIP_OPERATOR
#endif
```

Which honestly should be essentially the same


================
Comment at: libcxx/include/compare:250
 
+  friend constexpr bool operator==(partial_ordering, partial_ordering) noexcept = default;
+
----------------
cjdb wrote:
> miscco wrote:
> > I guess this is missing `_LIBCPP_INLINE_VISIBILITY `
> Does it need that? We've defined it here, so it should //already// have inline visibility.
In the worst case we improved the consitency with all other methods.


================
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);
 }
----------------
While I personally am in favor of reusing code I want to note that this carries some nonzero cost of creating a string_view.

This will almost certainly be optimized aways in RELEASE but will be noticeable in DEBUG.

I am kind of curios what the maintainers think about this. I know @BillyONeal avoids these miniscule pessimizations like the plague.


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

https://reviews.llvm.org/D80891