[libcxx-commits] [PATCH] D114912: [libc++] [P1614] Hidden-friend operator<=> for string_view.

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

Quuxplusone marked 2 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__string:334-336
+#if _LIBCPP_STD_VER > 17
+    typedef strong_ordering comparison_category;
+#endif
----------------
philnik wrote:
> Am I right that you use `typedef` instead of `using` for consistency here?
Yep, just consistency with lines 329–333.


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
philnik wrote:
> Your formatting seems a bit inconsistent in this file.
In what sense/where?
This is formatted the same as the rest of `libcxx/test/std/strings/string.view/string.view.comparison/` (which I just landed recently), so anything I change in here I'll also change in those 6 files.


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:95-101
+struct TraitsWithCompCat : std::char_traits<char> {
+    using comparison_category = std::partial_ordering;
+};
+
+struct TraitsWithoutCompCat : std::char_traits<char> {
+    void comparison_category();  // does not denote a type
+};
----------------
philnik wrote:
> > I recommend not inheriting from standard types in general.
> 
> Is there any reason this does not apply here?
Sheer laziness. ;) See `libcxx/test/support/constexpr_char_traits.h` for the amount of stuff that would have to be repeated here: it's about 100 lines. I'm vocally willing to spend 10 or even 20 lines to avoid inheritance (and on the average iterator it's like 5 lines), but somewhere between 20 and 100 lines I lose some of my enthusiasm. :)  Still, if someone shows how to eliminate this inheritance without adding  200 lines to this test, I'll be happy!

One idea to which I would be amenable is: Refactor `libcxx/test/support/constexpr_char_traits.h` into `libcxx/test/support/test_char_traits.h`, containing `cpp17_char_traits<T>` (which replaces `constexpr_char_traits` throughout the tests) and `cpp20_char_traits<T, Ordering>` (which replaces `TraitsWithCompCat` here, and in the tests for `string::operator<=>` once that's written). ... But even as I'm writing it out, it seems like a bad idea. `cpp20_char_traits<T, Ordering>` would be used in only 2 tests, so it doesn't pay for itself; and `TraitsWithoutCompCat` still needs to be written out longhand.

And I just realized that it might be nice to have another test for
```
struct TraitsWithUserDefinedCompCat : std::char_traits<char> {
    struct comparison_category {
        explicit comparison_category(std::strong_ordering);
    };
};
```
since for some reason C++20 doesn't require that `comparison_category` be a comparison category type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114912