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

Adrian Vogelsgesang via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Aug 10 13:32:38 PDT 2022


avogelsgesang accepted this revision.
avogelsgesang added a comment.

LGTM. Thanks!



================
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
----------------
Mordante wrote:
> 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.
makes sense


================
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:
> > 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.
Thanks for the pointer. Somehow, I missed that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131421



More information about the libcxx-commits mailing list