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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
Comment at: libcxx/include/string:4166
+#if _LIBCPP_STD_VER > 17
+    return basic_string_view<_CharT, _Traits>(__lhs) == basic_string_view<_CharT, _Traits>(__rhs);
+#else
----------------
we have that `basic_string_view` already available under the name `__self_view`

Here and other places.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp:40
+  AssertOrderAreNoexcept<T>();
+  AssertOrderReturn<Ordering, T>();
+
----------------
Mordante wrote:
> 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.)
>I assume you mean const CharT*.

right

> We usually don't test AssertOrderAreNotNoexcept; Standard libraries are allowed to mark functions noexcept when the Standard doesn't mandate it. [...]

Thanks for that clarification!

Afaict, `AssertOrderReturn<Ordering, T, CharT>()` still makes sense, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131421