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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added a comment.

Thanks for the review!



================
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.
----------------
avogelsgesang wrote:
> indentation seems off
yeah I noticed it to, already fixed.


================
Comment at: libcxx/include/string:4226
 
+#if _LIBCPP_STD_VER > 17
+
----------------
avogelsgesang wrote:
> 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
We don't have a real art/guidelines here. Personally I prefer to use the `#if _LIBCPP_STD_VER > XX` style.


================
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);
----------------
avogelsgesang wrote:
> afaict, the standard does not declare this as `noexcept`
You're right, good catch.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp:40
+  AssertOrderAreNoexcept<T>();
+  AssertOrderReturn<Ordering, T>();
+
----------------
avogelsgesang wrote:
> missing tests for the "mixed" comparison operators
> 
> AssertOrderAreNotNoexcept<T, CharT>
> AssertOrderReturn<Ordering, T, CharT>
> 
I assume you mean `const CharT*`.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp:40
+  AssertOrderAreNoexcept<T>();
+  AssertOrderReturn<Ordering, T>();
+
----------------
Mordante wrote:
> avogelsgesang wrote:
> > missing tests for the "mixed" comparison operators
> > 
> > AssertOrderAreNotNoexcept<T, CharT>
> > AssertOrderReturn<Ordering, T, CharT>
> > 
> I assume you mean `const CharT*`.
We usually don't test `AssertOrderAreNotNoexcept`; Standard libraries are allowed to mark functions `noexcept` when the Standard doesn't mandate it. MSVC STL uses this a lot, by adding that validation we would break this test for them. They are actively using our test to validate their implementation.

(Note that Standard libraries aren't allowed to mark functions `constexpr` when the Standard doesn't mandate it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131421