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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 3 inline comments as done.
Mordante 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
----------------
avogelsgesang wrote:
> we have that `basic_string_view` already available under the name `__self_view`
> 
> Here and other places.
I didn't know that. Still I prefer to keep the wording similar to that Standard. That makes it easier to verify we did the right thing. I like to use things like `__self_view` in places where the Standard specifies a behaviour without code.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp:40
+  AssertOrderAreNoexcept<T>();
+  AssertOrderReturn<Ordering, T>();
+
----------------
avogelsgesang wrote:
> 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.
Agreed, since `CharT` is defined on line 42 so that test has been placed on line 43.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131421