[libcxx-commits] [PATCH] D131421: [libc++] Uses operator<=> in string.

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added a comment.

Thanks for working on this!



================
Comment at: libcxx/include/string:53
     typedef streampos pos_type;
     typedef mbstate_t state_type;
+	using comparison_category = strong_ordering; // Since C++20 only for the specializations
----------------
also update the rest of the synopsis to `using` instead of `typedef`


================
Comment at: libcxx/include/string:54
     typedef mbstate_t state_type;
+	using comparison_category = strong_ordering; // Since C++20 only for the specializations
+	                                             // char, wchar_t, char8_t, char16_t, and char32_t.
----------------
indentation seems off


================
Comment at: libcxx/include/string:4194
 
+#if _LIBCPP_STD_VER < 20
 template<class _CharT, class _Traits, class _Allocator>
----------------
`_LIBCPP_STD_VER <= 17`? would be more consistent with the `_LIBCPP_STD_VER > 17` above...


================
Comment at: libcxx/include/string:4226
 
+#if _LIBCPP_STD_VER > 17
+
----------------
I would prefer to switch this `#if` around, such that the order of implementation matches the order in the synopsis. Not sure what the previous art/current guidelines are around this, though


================
Comment at: libcxx/include/string:4237
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+operator<=>(const basic_string<_CharT, _Traits, _Allocator>& __lhs, const _CharT* __rhs) noexcept {
+    return basic_string_view<_CharT, _Traits>(__lhs) <=> basic_string_view<_CharT, _Traits>(__rhs);
----------------
afaict, the standard does not declare this as `noexcept`


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp:40
+  AssertOrderAreNoexcept<T>();
+  AssertOrderReturn<Ordering, T>();
+
----------------
missing tests for the "mixed" comparison operators

AssertOrderAreNotNoexcept<T, CharT>
AssertOrderReturn<Ordering, T, CharT>



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131421