Page MenuHomePhabricator

[libc++] [compare] Named comparison functions, is_eq etc.
ClosedPublic

Authored by Quuxplusone on Sep 26 2021, 10:01 PM.

Details

Summary

Some of these were previously half-implemented in "ordering.h"; now they're all implemented, and tested.

Diff Detail

Event Timeline

Quuxplusone created this revision.Sep 26 2021, 10:01 PM
Quuxplusone requested review of this revision.Sep 26 2021, 10:01 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 26 2021, 10:01 PM
Quuxplusone added inline comments.Sep 27 2021, 9:15 AM
libcxx/include/__compare/is_eq.h
23–28

(1) Not marked _LIBCPP_HIDE_FROM_ABI, because I can't imagine their ABI will ever change, but I don't really understand the meaning of the marking, and maybe it's better to mark them; @ldionne opinion?

(2) TIL (again) that although constexpr on a global variable implies static, constexpr on a function implies inline. So it's correct that these header-defined functions aren't explicitly marked inline; they still have the right behavior.

Mordante accepted this revision as: Mordante.Sep 27 2021, 11:52 AM
Mordante added a subscriber: Mordante.

Would it be possible to handle the comment in test/std/ranges/range.adaptors/range.transform/iterator/compare.pass.cpp:39 in this commit/in a separate commit?

// TODO: when three_way_comparable is implemented and std::is_eq is implemented,

LGTM, but would like to see @ldionne's answer to _LIBCPP_HIDE_FROM_ABI since I think it should be added.

libcxx/include/__compare/ordering.h
341

Interesting only these 4 were implemented instead of all 6.

Overall, LGTM except the question with HIDE_FROM_ABI that is already raised and the small comment that I've left.

libcxx/include/__compare/is_eq.h
1

I would prefer to change this file to named_comparison_functions.h with the corresponding changes in guards and module.modulemap. From my perspective it gives better match to what section of the standard this private module implements.

Quuxplusone added inline comments.Sep 27 2021, 4:47 PM
libcxx/include/__compare/is_eq.h
1

I'm not strongly opposed to that idea. In fact I certainly would have done that, if these functions had actually been described in a named section like [named.comparison.functions]. Unfortunately, they're submarined into [compare.syn] with no English description anywhere in the standard (and compare_syn.h felt like the wrong answer).
Therefore, the recent-historical default behavior of "name the header after the entity it declares" came into play. I don't think anyone will be confused by how is_eq.h defines both is_eq and is_gt. And vice versa, if I'm looking for the header that defines is_gt, I'll sooner look in is_*.h than in named_*.h.

libcxx/test/libcxx/diagnostics/detail.headers/compare/is_eq.module.verify.cpp
16

@Mordante wrote:

Would it be possible to handle the comment in test/std/ranges/range.adaptors/range.transform/iterator/compare.pass.cpp:39 in this commit/in a separate commit?

In a separate PR, yes. That was actually how I stumbled onto this. It turns out that that test can't be merely uncommented; it needs some actual "design decision" stuff, so I want to do it in a separate PR. (Namely, ContiguousView has iterator type int*, so its iterators are three-way-comparable; which makes ThreeWayComparableView basically redundant. Whereas contiguous_iterator<int*> right now is not three-way-comparable, but I don't know that we should hard-code too many tests that rely on that being forever true. Anyway, separate PR, etc. :))

Rebase on main. Add _LIBCPP_HIDE_FROM_ABI, because I still don't know whether it's needed but at least it's consistent with everywhere else. @ldionne ping!

Mordante added inline comments.Sep 28 2021, 9:51 AM
libcxx/test/libcxx/diagnostics/detail.headers/compare/is_eq.module.verify.cpp
16

When it's indeed more work than uncommenting I also prefer a separate PR.

ldionne accepted this revision.Sep 29 2021, 12:55 PM

LGTM but please consider my inline suggestion!

Thanks a lot for the patch, the operator<=> support has been in need of some attention for a while.

libcxx/include/__compare/is_eq.h
23–28

Yes, we want _LIBCPP_HIDE_FROM_ABI here, like we do everywhere else. I agree in this case we could probably skip it since they are probably never going to change, but we might as well be consistent.

I wasn't aware of the constexpr implies inline bit. I find that surprising and weird. If it were me, I wouldn't rely on that corner case and I'd mark them as inline explicitly, since I expect most readers of the code will go "wut?" if we don't.

This revision is now accepted and ready to land.Sep 29 2021, 12:55 PM
Quuxplusone added inline comments.Sep 29 2021, 1:00 PM
libcxx/include/__compare/is_eq.h
23–28

If it were me, I wouldn't rely on that corner case and I'd mark them as inline explicitly, since I expect most readers of the code will go "wut?" if we don't.

Works for me! Marked.