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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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:
> ldionne wrote:
> > Mordante wrote:
> > > jloser wrote:
> > > > 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.
> > > About this remark and
> > > 
> > > > I'm claiming that libc++ should flex our vendor muscles and do it anyway, because the benefit is large and the cost is merely technical/pedantic.
> > > 
> > > How about filing an LWG-issue and propose a fix. Then add comment here it implements LWG-xxxx and its proposed resolution?
> > I would like us to file a LWG issue and see what the answer is *before* we implement these in terms of hidden friends. I understand the benefits and I probably agree, but I don't want us to do something if the Standard is going to go a different way, however wrong we might think the Standard is.
> > 
> > Until there's a LWG issue that has been accepted and the feeling in the room seems to be "yeah, let's make that change", I'd rather hold off with this patch.
> Can someone-not-me file that LWG issue, then?
> I feel pretty strongly that WG21 is never actually going to take the plunge until there's a vendor willing to stand up and say "We do that and it has been fine," and asking the question at this point is just going to get an automatic "no." (Kind of like `[[trivially_relocatable]]`. ;)) So I'd like the most senior-official person possible to make that LWG-issue request (looking at you @ldionne)... and I'd //also// still like us to Just Do It in libc++ regardless of WG21's answer.
> Can someone-not-me file that LWG issue, then?
> I feel pretty strongly that WG21 is never actually going to take the plunge until there's a vendor willing to stand up and say "We do that and it has been fine," and asking the question at this point is just going to get an automatic "no." 

I don't feel like that's the case. When libc++ (or someone who works on libc++) files an issue, usually I've seen it considered seriously, as it should be. Anyway, I don't mind doing it.

> and I'd also still like us to Just Do It in libc++ regardless of WG21's answer.

We have a hard stance on not being non-conforming, so I don't feel comfortable doing it. Especially since it will make us *subtly* non-conforming, which means we might end up with more issues down the road. For example, imagine the Standard decides to use some clever SFINAE to enable some function based on whether `x <=> y` is valid for some `x` and `y`. If we are not strictly conforming, we could end up breaking that API too by our non-conformity, and then we're in a more complex situation. That's one of the reasons why I've been advocating a hard stance on 0 nonconformity whenever we can.

[Extensions are a different thing in some cases. I'm also against extensions, however some extensions can't really have bad interactions with other parts of the library so they are more acceptable. I'm thinking about stuff like the `__int128_t` for random distributions we just did or adding `[[nodiscard]]` to some functions -- I can't easily imagine how those would lead us into subtle trouble down the road. This here feels like exactly the kind of thing that is going to lead us into trouble down the road.]



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

https://reviews.llvm.org/D114912