Page MenuHomePhabricator

[libc++] [P1614] Hidden-friend operator<=> for string_view.
Needs ReviewPublic

Authored by Quuxplusone on Dec 1 2021, 2:31 PM.

Details

Reviewers
ldionne
jloser
jdoerfert
Group Reviewers
Restricted Project
Summary

This implements *almost* conforming behavior for string_view's operator<=>.
I'm making it a hidden friend so that we can get rid of the "additional sufficient overloads" that were needed to deal with implicit conversions when it was a free function template. The Microsoft ABI can't deal with having multiple templates that instantiate to the same signature like that. (Microsoft works around their ABI by adding defaulted template parameters to the "sufficient overloads" so that they mangle differently. We could also do that, but I'm agitating to keep this simple at the cost of corner-case conformance.)

N.B.: I know this is (ever so slightly) non-conforming; 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.

The new test file is modeled on D114658.

Diff Detail

Event Timeline

Quuxplusone created this revision.Dec 1 2021, 2:31 PM
Quuxplusone requested review of this revision.Dec 1 2021, 2:31 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
philnik added a subscriber: philnik.Dec 1 2021, 3:02 PM
philnik added inline comments.
libcxx/include/__string
334–336

Am I right that you use typedef instead of using for consistency here?

libcxx/include/string_view
713
libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp
2

Your formatting seems a bit inconsistent in this file.

96–102

I recommend not inheriting from standard types in general.

Is there any reason this does not apply here?

Quuxplusone marked 2 inline comments as done.Dec 1 2021, 5:24 PM
Quuxplusone added inline comments.
libcxx/include/__string
334–336

Yep, just consistency with lines 329–333.

libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp
2

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.

96–102

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.

Quuxplusone marked an inline comment as done.Dec 1 2021, 5:35 PM
Quuxplusone added inline comments.
libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp
96–102

Update: What I wrote here doesn't work on the GCC buildkite anyway, because GCC and EDG have what-looks-like-a-bug: the compiler will "dig past" non-typenames to discover typenames in base classes. https://godbolt.org/z/qWxxoeE8r So I'll have to do something not-involving-inheritance here anyway! I don't have any good ideas what, though.

jloser added inline comments.Dec 1 2021, 5:38 PM
libcxx/include/__compare/comp_cat.h
28

What's _Dflt short for? Ditto below in the other alias templates.

34

Do we need to use void_t idiom if this is C++20-or-later code (can we use requires?)?

libcxx/include/string_view
29

Should we mention the note here that this is non-conforming? :)

757

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.

libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp
14

Nit: s/charT/CharT and s/traits/Traits to be consistent with libcxx code in string_view?

37

Since this test is run in C++20-or-later only, let's s/TEST_CONSTEXPR_CXX14/constexpr/g, please.

philnik added inline comments.Dec 2 2021, 2:50 AM
libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp
2

Ok, my wording was not exactly perfect. It's actually only the braces in lines 37 and 104. Everything else seems Ok.

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?

libcxx/include/__compare/comp_cat.h
28

I assume it means _Default. We should just use that name, it's clearer and those 3 additional characters don't cost a lot.

Quuxplusone marked 8 inline comments as done and an inline comment as not done.Dec 2 2021, 7:19 AM
Quuxplusone added inline comments.
libcxx/include/__compare/comp_cat.h
28

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

34

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.

libcxx/include/string_view
29

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.

757

Agreed; but this whole block is just moved (from old line 683), not modified, so I prefer to leave it as-was.

libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp
2

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.

14

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.

jloser added inline comments.Dec 2 2021, 7:49 AM
libcxx/include/string_view
29

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.

757

Got it - thanks for explaining.

libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp
14

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.

Quuxplusone marked 5 inline comments as done.

Review comments.
Eliminate inheritance from char_traits<char> (it wasn't that many lines after all, once I dropped unnecessary constexprness).
I'd missed "Mandates: R denotes a comparison category type," so now there's a static_assert with that message and a .verify.cpp test to verify the message.

Quuxplusone added inline comments.Dec 2 2021, 6:03 PM
libcxx/include/string_view
713

Oops, missed this one. Have now fixed it locally, so it'll be in the next diff uploaded and/or landed.

jloser added inline comments.Dec 3 2021, 8:49 AM
libcxx/include/string_view
716

Is the message more useful for developers using libc++ or implementors? I suspect the former, in which case I would mention something about traits​::​comparison_­category rather than R. I claim R is not useful for developers as that requires them to look at the Standard to figure out R means traits​::​comparison_­category. WDYT?

Quuxplusone marked 5 inline comments as done.Dec 3 2021, 10:42 AM
Quuxplusone added inline comments.
libcxx/include/string_view
716

Very fair point. I was kinda sneaking in the camel's nose of a new style, that whenever we insert a static_assert for literally no other reason than "the Standard says mandate this," we should make the message of the static_assert match the wording in the Standard. Which, if the reader is familiar with the Standard, should make it pretty obvious what's going on and why. But, you correctly observe that most readers won't be familiar with the Standard and so copying the Standard wording (while consistent (if we did it consistently)) isn't terribly helpful to the reader.

I don't mind changing this to either the more traditional

static_assert(__is_comparison_category_v<_Rp>, "Traits::comparison_category must be a comparison category type");

or the hybrid

static_assert(__is_comparison_category_v<_Rp>, "Mandates: Traits::comparison_category denotes a comparison category type");

Thoughts (from anyone at all)?

jloser added inline comments.Dec 3 2021, 10:59 AM
libcxx/include/string_view
716

I weakly prefer the hybrid:

static_assert(__is_comparison_category_v<_Rp>, "Mandates: Traits::comparison_category denotes a comparison category type");

I'd be fine with either of the two proposed different messages though.

Mordante added inline comments.
libcxx/include/string_view
29

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?

714
ldionne requested changes to this revision.Dec 6 2021, 7:37 AM
ldionne added inline comments.
libcxx/include/string_view
29

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.

This revision now requires changes to proceed.Dec 6 2021, 7:37 AM
Quuxplusone added inline comments.Dec 6 2021, 7:57 AM
libcxx/include/string_view
29

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.

ldionne added inline comments.Dec 6 2021, 8:20 AM
libcxx/include/string_view
29

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