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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review.



================
Comment at: libcxx/include/string:378
 template<class charT, class traits, class Allocator>
-bool operator==(const charT* lhs, const basic_string<charT, traits, Allocator>& rhs) noexcept;  // constexpr since C++20
+bool operator==(const charT* lhs, const basic_string<charT, traits, Allocator>& rhs) noexcept;  // constexpr since C++20, removed in C++20
 
----------------
philnik wrote:
> These shouldn't both be `constexpr` since C++20 and removed in C++20.
That's how it's implemented at the moment so I prefer to keep the synopsis as is. But we can remove the `_LIBCPP_CONSTEXPR_AFTER_CXX17` for the removed functions, that shouldn't affect the tests.

I'll make a followup for that.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp:9-11
+// Starting with C++20 the spaceship operator was included. This tests
+// comparison in that context, thus doesn't support older language versions.
+// These are tested per operator.
----------------
philnik wrote:
> Why do you explain why the test is marked unsupported C++03 - C++17? AFAIK we do that nowhere else.
We don't do that often, but in this case I think the explanation is useful. Somebody might expect all comparisions for all C++ versions in this test.

(In general I think we could have more comments in libc++ ;-) )


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.cmp/comparison.pass.cpp:78
+
+  return true;
+}
----------------
philnik wrote:
> You don't have to return true here. Just from the main test function.
Good point, I had only test before and refactored it to have an extra layer... and forgot to remove them here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131421