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 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
libcxx/include/__compare/is_eq.h
24–29

(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
326

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
2

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.

libcxx/include/__compare/is_eq.h
2

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
17

@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
17

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
24–29

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
libcxx/include/__compare/is_eq.h
24–29

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.