Some of these were previously half-implemented in "ordering.h"; now they're all implemented, and tested.
ldionne rarutyun Mordante
- Group Reviewers
- rG969359e3b86b: [libc++] [compare] Named comparison functions, is_eq etc.
(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.
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.
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.
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.
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).
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. :))
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.
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.
Works for me! Marked.