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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

@cjdb: I think you should go ahead and land the additions to "libcxx/test/std/strings/basic.string/string.nonmembers/" (other than ".../string_opcmp/", obviously). These look like good additions to the test suite, and should pass both before and after your spaceship-related changes. (It is mildly shocking that the old test suite lacked any tests where one operand was //not// a prefix of the other.)



================
Comment at: libcxx/include/string:4037
+template<class _CharT, class _Traits, class _Allocator>
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY
+bool
----------------
Either remove `[[nodiscard]]` (the easy path), or replace it with `_LIBCPP_NODISCARD_EXT` and put a libcxx/test/libcxx/ test on it.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp:13
+
+// we get this comparison "for free" because the string implicitly converts to the string_view
+
----------------
This comment suggests to me that we should have a new allocation-focused test that verifies that the comparison does no allocations (i.e. that we don't do it by converting the string_view to string, or anything equally dumb). And it should test all seven comparison operators in all modes. But this is largely orthogonal to this patch and could be done in a separate PR. I'll volunteer myself for it if you don't want it.

Separately, I think we //need// some tests for comparing `wstring` and maybe even `u8string`. As string comparison's relationship to `char_traits` gets more complicated, our existing lack of `wstring` tests becomes more glaring. This is //also// largely orthogonal to this patch, and would affect the existing tests for `<` `>` `<=` `>=` `==` `!=`.



================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp:24
+void
+test(const S& lhs, const S& rhs, std::weak_ordering x)
+{
----------------
I don't understand why you pass `strong_ordering::foo` but then implicitly convert it to `weak_ordering` here. Do we expect `string::operator<=>` to return `weak_ordering`? Why?
I suggest
```
template<class S, class O>
constexpr void test(const S& lhs, const S& rhs, O expected)
{
    auto actual = (lhs <=> rhs);
    ASSERT_SAME_TYPE(decltype(actual), decltype(expected));
    assert(actual == expected);
}
```


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp:59
+    {
+    typedef std::basic_string     <char, std::char_traits<char>, min_allocator<char>> S;
+    test(S(""), S(""), std::weak_ordering::equivalent);
----------------
Bizarre whitespace between `basic_string` and `<`. I guess it's for consistency with "string_string_view.pass.cpp" line 61; but I'd lose the whitespace in both cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895