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

[Christopher Di Bella via Phabricator via libcxx-commits]

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

In D80895#2751401 <https://reviews.llvm.org/D80895#2751401>, @Quuxplusone wrote:

> @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.

SGTM, gimme an hour or so.

> (It is mildly shocking that the old test suite lacked any tests where one operand was //not// a prefix of the other.)

If you think that's shocking, check out our container comparison tests.



================
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.
It's still unclear to me why `_LIBCPP_NODISCARD_EXT` is desirable over an attribute users can already disable.


================
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
+
----------------
Quuxplusone wrote:
> 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 `<` `>` `<=` `>=` `==` `!=`.
> 
> 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.

If you're concerned about `std::string_view(x)` doing an allocation: that should be a part of the `string_view` test suite, not the `string` comparison suite.
If you're concerned about an arbitrary allocation happening, then I'm not sure how we'd go about doing that in a feasible manner (why would we do this for comparison, and not `string::clear`, for example?).
Regardless of what you're wanting here, I'm happy to review when the time comes.

> 
> 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 `<` `>` `<=` `>=` `==` `!=`.
> 

This is a reasonable request, but since we don't have these tests for the other operators, I wonder if it'd be better to do this in a separate patch as one unit (i.e. after this patch lands, but not long after).


================
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)
+{
----------------
Quuxplusone wrote:
> 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);
> }
> ```
See below for a whole set of tests. The proposed change sounds reasonable to me: will fix in an hour or so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895