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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius marked an inline comment as done.
curdeius added inline comments.


================
Comment at: libcxx/include/__string:362
+
+#ifndef _LIBCPP_HAS_NO_SPACESHIP_OPERATOR
+    using comparison_category = strong_ordering;
----------------
Nit: please be consistent. Here you use `#ifndef _LIBCPP_HAS_NO_SPACESHIP_OPERATOR` and in other places you use `#if !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)` (e.g. line 469).


================
Comment at: libcxx/include/string_view:638
+template<class _CharT, class _Traits>
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
+auto operator<=>(basic_string_view<_CharT, _Traits> __lhs,
----------------
Nit: why using `_LIBCPP_CONSTEXPR_AFTER_CXX11` and not just `constexpr`?


================
Comment at: libcxx/include/string_view:652
+template<class _CharT, class _Traits>
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
+auto operator<=>(basic_string_view<_CharT, _Traits> __lhs,
----------------
As above, why `_LIBCPP_CONSTEXPR_AFTER_CXX11` and not `constexpr`?


================
Comment at: libcxx/include/string_view:653-654
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
+auto operator<=>(basic_string_view<_CharT, _Traits> __lhs,
+                 typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
+{
----------------
Should it be in the synopsis? Or rather, I don't think it should (according to http://eel.is/c++draft/string.view.synop), but why is this overload needed? Isn't `operator<=>(basic_string_view, basic_string_view)` enough?

Oh, I got my answer, http://eel.is/c++draft/string.view#comparison.


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/opcmp.string_view.pointer.pass.cpp:65-67
+    typedef std::basic_string_view<char, constexpr_char_traits<char> > SV;
+    constexpr SV sv1;
+    constexpr SV sv2{"abcde", 5};
----------------
Isn't it possible to factor out a test-all function from main, mark it `constexpr` and call it from both run- and compile-time context?

E.g.

```
template <...>
TEST_CONSTEXPR bool test(...) // as above but with constexpr and return bool.

TEST_CONSTEXPR bool test_all() // the first part of current main and return bool

main() {
  test_all();
  static_assert(test_all(), "");
}
```


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

https://reviews.llvm.org/D80891