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

Thu Dec 2 07:19:28 PST 2021

Quuxplusone marked 8 inline comments as done and an inline comment as not done.
Quuxplusone added inline comments.

================
Comment at: libcxx/include/__compare/comp_cat.h:27
+
+template<class _Traits, class _Dflt, class = void>
+struct __comp_cat {
----------------
ldionne wrote:
> jloser wrote:
> > What's `_Dflt` short for?  Ditto below in the other alias templates.
> I assume it means `_Default`. We should just use that name, it's clearer and those 3 additional characters don't cost a lot.
+1. I thought I remembered `_Dflt` being used elsewhere, but it's not; I see it only in the section heading [unique.ptr.dltr.dflt] and as a variable named `__dflt` in `<locale>`; meanwhile I see `_Default` in `<experimental/type_traits>`. I'll change it to `_Default`.

================
Comment at: libcxx/include/__compare/comp_cat.h:33
+template<class _Traits, class _Dflt>
+struct __comp_cat<_Traits, _Dflt, typename __void_t<typename _Traits::comparison_category>::type> {
+    using type = typename _Traits::comparison_category;
----------------
jloser wrote:
> Do we need to use `void_t` idiom if this is C++20-or-later code (can we use `requires`?)?
We have recently obsoleted `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR`, but we still guard things behind `_LIBCPP_HAS_NO_CONCEPTS`, so, no, `requires` wouldn't be as generally useful, quite yet.

================
Comment at: libcxx/include/string_view:29
     // 7.9, basic_string_view non-member comparison functions
+    // We make these hidden friends, in lieu of "sufficient additional overloads"
     template<class charT, class traits>
----------------
jloser wrote:
> Should we mention the note here that this is non-conforming? :)
And @ldionne wrote:
> Can you show me how that's non-conforming? I think you've explained it elsewhere but can you post a link if you have one?

The difference between "free template" and "hidden-friend non-template" is detectable by the user: in particular, this PR makes `three_way_comparable<reference_wrapper<string_view>>` true, where in libstdc++ it's false. (I consider this a good thing, though!)
https://godbolt.org/z/YK6KaarqW
https://quuxplusone.github.io/blog/2021/10/22/hidden-friend-outlives-spaceship/

Re mentioning here, e.g. inserting the word `non-conformingly` before `make`: I'm leery of adding code comments that reflect gripes about the current state of the Standard (or guesses about its intent), because they tend to bit-rot over time. I'd definitely mention it in my //commit message// — commit messages are for the ephemeral "news" about a change at-the-time-it-was-made — but my default behavior is not to put it in permanent code comments. Someone who wants to know the historical background of this line can always `git blame` or `git log` the file.

================
Comment at: libcxx/include/string_view:728
 inline constexpr bool ranges::enable_borrowed_range<basic_string_view<_CharT, _Traits> > = true;
 #endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)

----------------
jloser wrote:
> We can probably drop the comment here given how it's just 6 lines up. Note in the next `#if` block below (`#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES))`, we omit the comment since it's short like this one.
Agreed; but this whole block is just moved (from old line 683), not modified, so I prefer to leave it as-was.

================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
philnik wrote:
> Quuxplusone wrote:
> > 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.
> Ok, my wording was not exactly perfect. It's actually only the braces in lines 37 and 104. Everything else seems Ok.
Ah: function braces uncuddled, class-definition braces cuddled. I see. (I was never gonna notice that on my own without that extra information. :))  I don't particularly care either way — I just did what came naturally to my fingers (which I'm sure is influenced by what I see, and vice versa what I type influences the ratios of what's checked in, over time). I don't think we have much of a consistent house style here — but both "function braces uncuddled" and "class-definition braces cuddled" seem to be in the numerical majority.
```
$ git grep 'int main(.*)$' libcxx/test/ | wc -l
    5353
$ git grep 'int main(.*) {' libcxx/test/ | wc -l
    1381
$ git grep 'int main(.*) {.*}$' libcxx/test/ | wc -l
     123
```
versus
```
$ git grep 'struct .* {$' libcxx/test/ | wc -l
    2277
$ git grep 'struct .*[a-zA-Z0-9_]$' libcxx/test/ | wc -l
    1171
```
So I'm inclined to do nothing here, unless a strong preference for some-specific-rule emerges.

================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:13
+
+// template<class charT, class traits>
+//   constexpr auto operator<=>(basic_string_view<charT, traits> lhs, basic_string_view<charT, traits> rhs);
----------------
jloser wrote:
> Nit: `s/charT/CharT` and `s/traits/Traits` to be consistent with `libcxx` code in `string_view`? 
IIRC we had a roughly isomorphic nit session (instigated by me ;)) about `span::extent`, where the template parameter is named `Extent` but exposed publicly and sometimes used in signatures as `extent`, which is inconsistent and annoying. Here, similarly, https://eel.is/c++draft/string.view#template spells the template parameters as `charT, traits`, and so sometimes we "correct" it to `CharT, Traits` and usually we don't. (And in a few tests' comments we say `_CharT`.)  Anyway, `charT` is consistent with
`libcxx/test/std/strings/string.view/string.view.io/` and almost all of `libcxx/test/std/strings/basic.string/`, among others, so I'm inclined to just leave it as-is.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114912