[libcxx-commits] [PATCH] D80895: [libcxx] adds operator<=> for basic_string

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius marked an inline comment as done.
curdeius added inline comments.


================
Comment at: libcxx/include/string:362
 template<class charT, class traits, class Allocator>
-bool operator==(const charT* lhs, const basic_string<charT, traits, Allocator>& rhs) noexcept;
+bool operator==(const basic_string<charT,traits,Allocator>& lhs, const charT* rhs) noexcept;
 
----------------
You changed the order of parameters and now this overload is the same as the one below.
And BTW, why removing spaces in `<...>`? :)


================
Comment at: libcxx/include/string:365
 template<class charT, class traits, class Allocator>
-bool operator==(const basic_string<charT,traits,Allocator>& lhs, const charT* rhs) noexcept;
+bool operator==(const basic_string<charT,traits,Allocator>& lhs, const charT* rhs) noexcept; // C++17
 
----------------
I think you should mark the overloads removed in C++20 as `removed in C++20` or `until C++17` instead of just `C++17` as the latter is misleading.


================
Comment at: libcxx/include/string:418
 template<class charT, class traits, class Allocator>
-bool operator>=(const charT* lhs, const basic_string<charT, traits, Allocator>& rhs) noexcept;
+bool operator<=>(const basic_string<charT, traits, Allocator>& lhs,
+                 const basic_string<charT, traits, Allocator>& rhs) noexcept; // C++20
----------------
Return type is not `bool` -> `see below` in standard.


================
Comment at: libcxx/include/string:422
+template<class charT, class traits, class Allocator>
+operator<=>(const basic_string<charT, traits, Allocator>& lhs, const charT* rhs) noexcept; // C++20
 
----------------
This overload doesn't seem to be marked `noexcept` by the standard: http://eel.is/c++draft/string.syn.


================
Comment at: libcxx/include/string:422
+template<class charT, class traits, class Allocator>
+operator<=>(const basic_string<charT, traits, Allocator>& lhs, const charT* rhs) noexcept; // C++20
 
----------------
curdeius wrote:
> This overload doesn't seem to be marked `noexcept` by the standard: http://eel.is/c++draft/string.syn.
Where is the return type? Standard marks it as `see below`.


================
Comment at: libcxx/include/string:3970
+operator==(const basic_string<_CharT, _Traits, _Allocator>& __lhs,
+           const basic_string<_CharT, _Traits, _Allocator>& __rhs) _NOEXCEPT
+{
----------------
I think you should just use `noexcept` instead of `_NOEXCEPT` as it should be always available in C++20.


================
Comment at: libcxx/include/string:4008
+operator==(const basic_string<_CharT, _Traits, _Allocator>& __lhs,
+           const basic_string<_CharT, _Traits, _Allocator>& __rhs) _NOEXCEPT
+{
----------------
This `_NOEXCEPT` should be kept as is of course.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp:35
+    typedef std::string S;
+    test("", S(""), std::strong_ordering::equal);
+    test("", S("abcde"), std::strong_ordering::less);
----------------
cjdb wrote:
> curdeius wrote:
> > I may be missing something, but it seems that there are no tests for the basic cases like:
> > "abc" < "acb"
> > "cba" > "aaa"
> > All (non-equal) tests fall into the shorter/longer check.
> Done. I also added these tests to the inequality tests.
Great!


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp:14
+// template<class charT, class traits, class Allocator>
+//   bool operator<=>(const charT* lhs, const basic_string<charT,traits,Allocator>& rhs);
+
----------------
`bool`?


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

https://reviews.llvm.org/D80895