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

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
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>
----------------
Quuxplusone wrote:
> 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.
SGTM. I'm OK with it as a commit message only.

@ldionne the `string_view` comparison functions are in the "Non-member comparison functions" section (https://eel.is/c++draft/string.view.comparison). This is the same for some other class templates that have this "issue" (i.e. they could be written in terms of hidden friends, but aren't in the standard). The benefit gain for `string_view` is more-so than other class templates due to the additional two overloads per comparison operator for the "convertible to string_view in a non-deducible context" case. Those overloads go away if we just move to hidden friends.


================
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)
 
----------------
Quuxplusone wrote:
> 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.
Got it - thanks for explaining.


================
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);
----------------
Quuxplusone wrote:
> 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.
Ah yes, I recall that now regarding using the value from the template or the `static constexpr` value from the span PR. I'll just sigh and say leave it as-is given the precedence. :)

Thanks for explaining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114912