[libcxx-commits] [PATCH] D80895: [libcxx] adds consistent comparison for `basic_string`

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Do these changes require updating the status of papers?



================
Comment at: libcxx/include/string:4037
+template<class _CharT, class _Traits, class _Allocator>
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY
+bool
----------------
Quuxplusone wrote:
> Either remove `[[nodiscard]]` (the easy path), or replace it with `_LIBCPP_NODISCARD_EXT` and put a libcxx/test/libcxx/ test on it.
The `[[nodiscard]]` is an extension, can you the `_LIBCPP_NODISCARD_EXT` macro and add it to the list extensions.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp:31
+
+int main(int, char**)
+{
----------------
Can you turn the test into a templated test and test with all allowed string types? 
The support header `make_string.h` has some macros that will be useful for these tests.


================
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);
+
----------------
curdeius wrote:
> `bool`?
Please address the comment above.


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


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp:12
+// <string>
+
+// we get this comparison "for free" because the string implicitly converts to the string_view
----------------
Please add the function being tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895