[libc++][ranges] Add range.cmp: equal_to, not_equal_to, less, etc.

[zoecarver]

Summary

Adds the six new concept constrained comparisons.

[zoecarver]

I should have mentioned: this implements LWG3530.

libcxx/include/functional

3226

@ldionne want me to break this out into its own header?

libcxx/test/std/utilities/function.objects/range.cmp/less.pass.cpp

23

Can anyone think of a type for this? If not, I'll just remove the comment.

[cjdb]

LGTM overall, thanks for working on this :-)

libcxx/include/functional

3232

Please make these [[nodiscard]].

3237

Don't we typically make these void? Having said that, I'm all for giving them all a different type.

libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp

32

I find static_asserting a requires-expression to be a lot clearer when it comes to checking that a typename is present.

libcxx/test/std/utilities/function.objects/range.cmp/not_equal_to.pass.cpp

22

This should be named NotEqualityComparable. Also, although operator!= isn't generated, I find it very obscure as to why, so would you mind deleting operator!= please?

libcxx/test/support/pointer_comparison_test_helper.h

27–28

Do we need to have loops in tests?

[• Quuxplusone]

libcxx/test/std/utilities/function.objects/range.cmp/less.pass.cpp

23

IIUC, https://cplusplus.github.io/LWG/issue3530 seems to be talking about the "a < b resolves to builtin pointer comparison" nonsense that spawned about half of https://quuxplusone.github.io/blog/2019/01/20/std-less-nightmare/ . No library actually implements that issue's BUILTIN-PTR-THREE-WAY because it's literally not implementable without compiler magic. But if you did want a test for LWG3530, just for completeness, just to prove that libc++ doesn't implement the unimplementable magic ;) then you would write something like this:

struct A {
    operator int*() const;
    friend bool operator>=(A, A) = delete;
};

Here A a; a <=> a is well-formed and resolves to built-in pointer comparison... and yet, a >= a is ill-formed. So LWG3530 is saying that actually std::three_way_compare()(a, a) should also be ill-formed.
https://godbolt.org/z/KxWEa7q3s
This is of course absolutely crazy and not at all in keeping with the "STL classic" philosophy of lifted operators like std::less, std::equal_to, etc.; the extra syntactic constraints seem to have leaked over from Ranges. But unless we're going to file a library defect against the resolution of LWG3530, I think we're stuck implementing it.

26

Use hidden friends; they're harder to screw up. Here I assume leaving off the const was accidental, right?

[• Quuxplusone]

libcxx/include/functional

3287

Use using is_transparent = void;, for consistency.
Also, please add the missing _VSTD:: to all instances of forward in the new stuff.

[zoecarver]

• Update based on review.
• Use correct unsupported comment for tests.

[zoecarver]

libcxx/test/support/pointer_comparison_test_helper.h

27–28

I'd rather not update these tests. Also, I don't think this (albeit nested) loop is adding too much time.

[cjdb]

libcxx/test/support/pointer_comparison_test_helper.h

23–24

This can be made declarative using std::generate_n, and std::make_shared<T>() will make it clearer that there's no memory leaks.

27–28

Wanting to remove loops from tests isn't to do with run-time; it's to do with knowing the test is correct on inspection. The less logic that's in a test, the easier it is to verify that the test itself doesn't have a bug in it.

I'd rather not update these tests

I'm confused. Didn't you just add this file?

[zoecarver]

libcxx/test/support/pointer_comparison_test_helper.h

23–24

Good suggestion, thanks. Not sure how it makes it clearer that there are no leaks, but it does make it much more readable, so I'll incorporate it.

27–28

I'm confused. Didn't you just add this file?

No, I moved it from libcxx/test/std/utilities/function.objects/comparisons/pointer_comparison_test_helper.h.

Wanting to remove loops from tests isn't to do with run-time; it's to do with knowing the test is correct on inspection. The less logic that's in a test, the easier it is to verify that the test itself doesn't have a bug in it.

So, would you rather I only test ~8 pointers? Even then we'd add at least 48 lines to the file, and we'd be significantly reducing the coverage. I think in the case of any loop, really, there isn't much readability lost for that logic. If this were in a helper function, or in an overloaded class template, that might be a different story, but I think everyone here is able to flatten a loop mentally.

Also, I'd like to point out that this test isn't necessarily suppose to be easily verifiable on inspection; think about it more as a stress test or something. It's testing these operators on lots of different pointers as uintptr_t and void*.

Think about it like the concept tests: I'm fine if we want to have a lot of complexity to stress test the compiler with hundreds or thousands of "generated" templates/macros. But let's keep those separate, and have the other, easily verifiable tests as well. That's why this is in a different file too.

[cjdb]

LGTM

libcxx/test/support/pointer_comparison_test_helper.h

23–24

Whenever I see naked new, my eyes scan for a corresponding delete. It's not till after I can't find delete that my brain realises one might be constructing a smart pointer!

27–28

Ah, it's stress testing, not correctness testing. Carry on then.

[zoecarver]

• Use generate_n and make_shared.

[• Quuxplusone]

libcxx/test/support/pointer_comparison_test_helper.h

23–24

+1 on "no raw new and delete." However, -1 on writing std::make_shared<T> without calling it (i.e. relying on conversion to function pointer); I'm not sure that's conforming with SD-8 and it's certainly not in the spirit of SD-8. To get the same behavior simpler, I'd suggest

for (auto& ptr : pointers) {
    ptr = std::make_shared<T>();
}

However, I also don't understand why you need shared ownership of T objects here at all. Am I correct that all you're doing with these various heap-allocated objects is comparing their addresses over and over? And it doesn't matter what T is at all? This feels mildly silly. I would suggest simply making an array

int a[2];
int *lhs = &a[0];
int *rhs = &a[1];
auto lhs_uint = reinterpret_cast<std::uintptr_t>(lhs);
auto rhs_uint = reinterpret_cast<std::uintptr_t>(rhs);
assert(comp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));
assert(vcomp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));

Bonus: try with int *ths = nullptr; as well.

[cjdb]

libcxx/test/support/pointer_comparison_test_helper.h

23–24

Alternatively (preferred): put make_shared in a lambda.

Alternatively, alternatively: we are the implementation and if we break ourselves, that's something we need to fix down the road.

[zoecarver]

• Update based on review.

[zoecarver]

libcxx/test/support/pointer_comparison_test_helper.h

23–24

Updated this to be a static size array. Good call. No need for the template/shared_ptr.

[zoecarver]

@Quuxplusone look good now?

[curdeius]

In D100429#2687362, @zoecarver wrote:

I should have mentioned: this implements LWG3530.

Should it be marked on status page?

[zoecarver]

In D100429#2692729, @curdeius wrote:

In D100429#2687362, @zoecarver wrote:

I should have mentioned: this implements LWG3530.

Should it be marked on status page?

It hasn't been officially accepted yet, so it's not in the status page. Otherwise, yes, you're right.

[• Quuxplusone]

libcxx/include/functional

3233

Nit: _Tp&& __t, not _Tp &&__t, throughout.

3244

@ldionne, apparently we're free to use either expression x != y or !(x == y) here (because the whole point of concept equality_comparable_with is that those two expressions are interchangeable). I have a moderate preference for x != y, for simplicity and consistency with the ordinary std::not_equal_to — "we mean not-equal so let's write not-equal." What's your take? (See also my next comment below.)

https://en.cppreference.com/w/cpp/utility/functional/ranges/not_equal_to

3254

Here I have a strong preference that ranges::greater should be implemented via t > u, not u < t, because the latter just looks like a typo and I'm worried someone will "fix" it under maintenance.

[• Quuxplusone]

libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp

27–32

Nit: Please remove these two instances of constexpr, and also remove all other constexpr that aren't necessary to the point of the test. Remember, in general, concepts care only whether some expression e.g. x == y is well-formed; they don't actually try to evaluate the expression at compile-time. (And so we should save the 10 characters. But also, weakly-arguably, removing constexpr makes the test stronger because now we're kinda "verifying" that nothing in libc++ is trying to evaluate x == y at compile time.)

37–38

Drive-by FYI: the test class explicit_operators provides seven comparison operators (both the usual six and a redundant operator<=>). I'm not sure that was intentional.

libcxx/test/support/compare_types.h

8–9

Probably a good idea to fix the include guard's name.

578–580

Nit: please fix the whitespace to match lines 574-577, i.e., { return false; } all on the same line. Ditto below.

libcxx/test/support/pointer_comparison_test_helper.h

23–24

This loop now runs 100x100 times, and doesn't actually test anything after the first two iterations.
I see you are now testing 3 of the 4 cases here:

• valid pointer versus itself
• valid pointer versus another valid pointer
• null pointer versus null pointer

but you aren't testing

• valid pointer versus null pointer

Is that because you think it's undefined behavior? I don't know whether it is or not. But anyway, I think the structure of this function ought to reflect the fact that there are 4 cases to test — not, like, 100x100 cases.

[zoecarver]

• Apply most of Arthur's comments.

[zoecarver]

libcxx/include/functional

3244

I don't really care which thing we do. As for this comment and the one below, I'd be interested to hear other's opinions as well. I'm happy to implement whatever the consensus is.

libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp

37–38

I'm pretty sure it wasn't. I removed the spaceship operator. Not that it matters too much which are removed, but I think explicit_operators sort of indicates that the individual operators are declared.

cc @cjdb

[zoecarver]

libcxx/test/support/compare_types.h

27

If all these changes are distracting, I can move and format this file in a separate nfc patch.

[cjdb]

libcxx/include/functional

3243

noexcept specifiers would be appreciated.

3244

Per range.cmp, not_equal_to::operator() has effects equivalent to return !ranges::equal_to{}(std::forward<T>(t), std::forward<U>(u)).

I'd actually prefer to see all of the implementations return the expressions in the standard wording, verbatim.

libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp

37–38

but I think explicit_operators sort of indicates that the individual operators are declared.

Yes, this was intentional.

[• Quuxplusone]

libcxx/test/std/utilities/function.objects/range.cmp/greater_equal.pass.cpp

34

Btw, if this test still compiles after making is_transparent equal to void, something is a little screwy somewhere. I recommend

static_assert(requires { typename std::ranges::greater_equal::transparent; });

as you've got elsewhere. (And apply consistently throughout.)

41

This comment is more confusing than other comments. (I recommend deleting it.)
Also, consider checking heterogeneous comparisons: fn(2, 1L) and maybe even fn(2, MoveOnly(1)) if the latter compiles (I'm not sure off the top of my head whether we expect it to or not, nor whether that's interesting).

libcxx/test/std/utilities/function.objects/range.cmp/not_equal_to.pass.cpp

28

remove constexpr here

libcxx/test/support/compare_types.h

27

FYI, no objection from me if you want to push a whitespace-only cleanup. (But I don't find these changes particularly distracting, either.)
Or, give me the go-ahead and I'll do it.

57–58

I do wonder, in passing, (A) what ec1 stands for, and (B) why we provide an operator!= here when we already have a suitable operator==.
Also, in passing, none of these should be noexcept. (Removing that keyword in the first place would have shortened the line enough to escape the ravages of clang-format, and then we wouldn't even be looking at this code anymore.)

[cjdb]

libcxx/test/support/compare_types.h

57–58

(A) what ec1 stands for

equality_comparable1. I didn't expect people would like equality_comparable_with_equality_comparable1. Perhaps control would be a more descriptive name.

(B) why we provide an operator!= here when we already have a suitable operator==.

Many types that predate C++20 have operator!=.

Also, in passing, none of these should be noexcept.

noexcept is fine, unless you can provide a technical reason why it "shouldn't" be there.

[tcanens]

libcxx/include/functional

3244

I've removed the claim on cppreference that this can be implemented with !=; the convertible-to-pointer corner case means that it can't - consider a case where == results in a built-in comparing pointers but != doesn't; we attach no semantic requirements to the != in such a case.

[• Quuxplusone]

libcxx/include/functional

3244

@tcanens: Uh-oh. And the resolution of https://cplusplus.github.io/LWG/issue3530 doesn't affect your statement?
Could you post an example test case where the difference between !ranges::equal_to{}(FWD(t), FWD(u)) and FWD(t) != FWD(u) is observable?
And I would bet that the difference between !ranges::equal_to{}(FWD(t), FWD(u)) and !(FWD(t) == FWD(u)) is also observable, because the latter might be applying ! to a type other than bool?

[cjdb]

libcxx/include/functional

3244

And the resolution of https://cplusplus.github.io/LWG/issue3530 doesn't affect your statement?

No, because that's exclusively to do with pointers.

Could you post an example test case where the difference between !ranges::equal_to{}(FWD(t), FWD(u)) and FWD(t) != FWD(u) is observable?

enum class X { a, b };

[[nodiscard]] bool operator!=(X const x, X const y) noexcept
{
  auto const result = static_cast<int>(x) != static_cast<int>(y);
  std::cout << result << '\n';
  return result;
}

Same for t > u below.

And I would bet that the difference between !ranges::equal_to{}(FWD(t), FWD(u)) and !(FWD(t) == FWD(u)) is also observable, because the latter might be applying ! to a type other than bool?

Sadly. I think !bool(FWD(t) == FWD(u)) is okay because FWD(t) == FWD(u) needs to be converted to bool in ranges::equal_to{}(FWD(t), FWD(u)) anyway.

[tcanens]

libcxx/include/functional

3244

struct T {
    operator void*() const { return nullptr; }
    friend bool operator!=(T, T) { return true; }
};

LWG3530 means that implementations don't have to use heroic efforts to detect and support types for which the other comparisons are ill-formed, but here the comparison is well-formed and "just" gives nonsensical results. It doesn't seem to be worth the extra specification effort to make this pathological case undefined, since the comparison function objects are the only ones affected, and they still produces meaningful results.

And I would bet that the difference between !ranges::equal_to{}(FWD(t), FWD(u)) and !(FWD(t) == FWD(u)) is also observable, because the latter might be applying ! to a type other than bool?

boolean-testable has enough semantic requirements for the latter to work (and for the carve-out case the result type is definitely bool). I would consider the difference unobservable.

[tcanens]

libcxx/include/functional

3244

I think the enum example goes a bit too far. For types required to model equality_comparable (or even the weaker form), I'm pretty sure the intent is that implementations can use x != y and !(x ==y) mostly interchangeably.

[zoecarver]

• Apply feedback
• Add noexcept
• Add tests for heterogeneous types

[zoecarver]

libcxx/include/functional

3244

Okay so @cjdb wants the wording from the standard verbatim and @Quuxplusone wants !=. Again, I don't feel strongly, so unless there's a consensus (or a majority who strongly prefers one) I'm going to leave this as-is.

[cjdb]

LGTM pending feedback resolution.

libcxx/include/functional

3228–3229

Perhaps we should put this in __detail/noexcept_helpers.h?

3244

We can't use != (similarly for <=, >=, and >). I don't mind using == and < directly since you're using _LIBCPP_NOEXCEPT_RETURN, but if you'd prefer to make a helper variable (e.g. __is_less_nothrow<T, U>), then going with the standard wording is probably better.

[• Quuxplusone]

libcxx/include/functional

3228–3229

If it ends up being used in more than one place, we might want to pull it out (or else stop using it). For now, it's good to follow the general principle of "declare as close as possible to the point of use."

3232

I like [[nodiscard]] here. But shouldn't we be expressing it via _LIBCPP_NODISCARD_EXT?
@ldionne, thoughts?

If [[nodiscard]] (or the spelling thereof) is not immediately a no-brainer for @ldionne, then I suggest leaving it off for now, and applying _LIBCPP_NODISCARD_EXT consistently across the entire <functional> library in a separate PR. For example, right now std::less<int>::operator() is not-nodiscard; I think we probably all agree that it should be nodiscard; but that'd be an extension and we'd want to use the right macro and we'd want tests for it.

3244

Leaving as-is is good with me. The fewer constrained function templates we instantiate, and the fewer dependencies-on-other-functions we introduce, the better. :)
However, before landing, please do change it to use != locally and verify that at least one of your test cases "successfully fails." (IOW, I believe Tim when he says we can't use !=; but if it's true we can't use !=, then we should have a regression test for that. We shouldn't land this without some such "successfully failing" test.)

[zoecarver]

libcxx/include/functional

3228–3229

@Quuxplusone it's also used in D100255 and <tuple>. __detail/noexcept_helpers.h sounds good to me. But maybe there's a more general place we could start putting stuff like this. (A place we could also move some stuff to, mainly from type_traits.)

3244

So, I'm adding a test for

struct OddType {
    constexpr operator void*() const { return nullptr; }
    friend constexpr bool operator!=(OddType, OddType) { return true; }
};

IIUC the pointer exception gives us the ability to have this type (where != isn't the same as !(==)). However, this is where it gets fun... because if this type is supported (and if I'm writing a test for it) then the Standard says std::ranges::not_equal_to()(OddType(), OddType()) should be false, even though the != operator unconditionally returns true. I'm happy to add that test, but I don't see a lot of benefit in it, and it seems very confusing. I think "ignoring" this case, or filing an LWG issue would be a better course of action.

[zoecarver]

• Apply review feedback.
• Rebase.

[zoecarver]

libcxx/include/__detail/noexcept_helpers.h

25 ↗

(On Diff #338993)

We never get a chance to #undef this now but I think that's OK.

[zoecarver]

libcxx/include/functional

3244

LWG3530 means that implementations don't have to use heroic efforts to detect and support types for which the other comparisons are ill-formed, but here the comparison is well-formed and "just" gives nonsensical results. It doesn't seem to be worth the extra specification effort to make this pathological case undefined, since the comparison function objects are the only ones affected, and they still produces meaningful results.

@tcanens LWG3530 unconditionally constrains not_equal_to with equality_comparable_with which semantically requires that (!(x == y)) == (x!=y). Doesn't that mean T isn't a valid argument in your example?

[tcanens]

libcxx/include/functional

3244

"satisfies" is purely syntactic, and that's intentional. The semantic requirements are imposed through "models".

LWG3530 is careful to not impose the equality_comparable_with (and similar) semantic requirements on things that reduce to built-in pointer comparisons because the whole point of the special case is to produce a consistent total order even when the built-in comparison doesn't.

In some cases with raw pointers, the core language permits x == y and x != y to both produce true (or both false); the example isn't really behaviorally distinguishable from such a case, and I don't think it's worth spending LWG's time crafting wording trying to ban it.

[zoecarver]

libcxx/include/functional

3244

OK. I get it. I was confused because of the Constraints: T and U satisfy equality_comparable_with. But I understand now, T and U do satisfy equality_comparable_with, it's just the built-in pointer that the operator reduces them to which doesn't.

So, you don't think it's worth LWG's time to "fix" this so that in your example the != operator is selected? I was going to write up an email to the reflector, but if you don't think it makes sense to spend time on it, then I won't.

[tcanens]

libcxx/include/functional

3244

OK. I get it. I was confused because of the Constraints: T and U satisfy equality_comparable_with. But I understand now, T and U do satisfy equality_comparable_with, it's just the built-in pointer that the operator reduces them to which doesn't.

The built-in pointers satisfy equality_comparable_with (but do not model it unless restricted to the domain on which the comparisons have well-defined results by core language rules).

So, you don't think it's worth LWG's time to "fix" this so that in your example the != operator is selected? I was going to write up an email to the reflector, but if you don't think it makes sense to spend time on it, then I won't.

I definitely don't want != to be selected in that example. That breaks the guarantee that ranges::equal_to(x, y) == !ranges::not_equal_to(x, y) which is far more critical.

[zoecarver]

libcxx/include/functional

3244

OK. Thanks for explaining. In that case, I guess this is just a weird type, and the behavior is as correct as it can/should be.

[zoecarver]

• Add test for PtrAndNotEqOperator.

[tcanens]

libcxx/include/functional

3235

The implicit conversion to bool is not captured in the noexcept, ditto elsewhere.

libcxx/test/support/pointer_comparison_test_helper.h

13

If you want to test this sort of thing, I'd suggest something like the example from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78420#c0. "Comparison produces a total order when the built-in is required to" (which describes 3/4 of the test cases below) is not a particularly interesting property compared to "comparison produces a total order even when the built-in is not required to".

[• Quuxplusone]

libcxx/include/__detail/noexcept_helpers.h

25 ↗

(On Diff #338993)

Yes, that part is okay.
Based on @tcanens' repeated observations that this macro is being misused to create the wrong noexcept-specifiers, though, I think we ought to put a moratorium on this kind of macro. It's not really saving us any work, in the sense that it's actually slowing down reviews by making us have to skip back and forth to compare the function body with the definition of the macro, and then find out that the macro is doing the wrong thing in this case, and so on. I think noexcept-clauses should just be written out in the usual way, and this macro should be retired.

Bonus: no more __detail/ dumping-ground directory. Bonus: no more noexcept_helpers.h (plural name, only one "helper", helper doesn't help).

libcxx/test/support/pointer_comparison_test_helper.h

13

How about

struct {
  int a, b;
} local;
int *a = &local.a;
int *b = &local.b;
int *c = nullptr;
int *d = &local.a + 1;
std::uintptr_t ua = reinterpret_cast<std::uintptr_t>(a);
std::uintptr_t ub = reinterpret_cast<std::uintptr_t>(b);
std::uintptr_t uc = reinterpret_cast<std::uintptr_t>(c);
std::uintptr_t ud = reinterpret_cast<std::uintptr_t>(d);
assert(comp(a, a) == ucomp(ua, ua));
assert(comp(a, b) == ucomp(ua, ub));
assert(comp(a, c) == ucomp(ua, uc));
assert(comp(a, d) == ucomp(ua, ud));
assert(comp(b, a) == ucomp(ub, ua));
assert(comp(b, b) == ucomp(ub, ub));
assert(comp(b, c) == ucomp(ub, uc));
assert(comp(b, d) == ucomp(ub, ud));
assert(comp(c, a) == ucomp(uc, ua));
assert(comp(c, b) == ucomp(uc, ub));
assert(comp(c, c) == ucomp(uc, uc));
assert(comp(c, d) == ucomp(uc, ud));
assert(comp(d, a) == ucomp(ud, ua));
assert(comp(d, b) == ucomp(ud, ub));
assert(comp(d, c) == ucomp(ud, uc));
assert(comp(d, d) == ucomp(ud, ud));

I haven't thought hard about how to factor out the testing of std::less<int*> and std::less<> so that you can just call the function template twice with different args instead of repeating that entire 16-line array of tests for vcomp.

[• Quuxplusone]

libcxx/include/functional

510–511

(Now moot, but just for your future information) libc++ sorts _ asciibetically prior to a:
__config __detail concepts type_traits typeinfo is correct,
concepts __config __detail typeinfo type_traits would be wrong.

[zoecarver]

• Fix noxecpt (remove header)
• Re-write pointer_comparison_test_helper.h again.

libcxx/test/support/pointer_comparison_test_helper.h

13

The loop is back. But in this case, think it both increases coverage and readability. Let me know what you think.

[zoecarver]

libcxx/test/support/pointer_comparison_test_helper.h

5

No need for std::vector or memory to be included here.

[zoecarver]

• Remove unused includes.

[• Quuxplusone]

libcxx/test/support/pointer_comparison_test_helper.h

26–29

I don't mind the loop now; but please use the values I gave:

struct {
  int a, b;
} local;
int *pointers[] = { &local.a, &local.b, nullptr, &local.a + 1 };

The reason for these four values in particular is:

• &a < &b is true in practice but unspecified in theory (AFAIK) (because a and b are not members of the same array).
• &a < &a+1 is true in both practice and theory
• &b == &a+1 is UB in practice but unspecified in theory

(&b and &a+1 are the same memory address in practice; but compilers including GCC and perhaps Clang will do front-end optimizations under the assumption that a pointer derived from a cannot possibly alias a pointer derived from b. So in practice you get inconsistent results; see my blog post linked earlier. But, for our purposes here, as @tcanens said, this is the sole really interesting case. Certainly it is uninteresting to compare &buff[0] with &buff[1].)

[zoecarver]

• Use Arthur's values.

[zoecarver]

Okay, the builds passed (after only 11 hours). @Quuxplusone how's this looking to you?

[• Quuxplusone]

libcxx/test/std/utilities/function.objects/range.cmp/less.pass.cpp

61

BLOCKER: Did you mean /std::less/std::ranges::less/? But then that won't compile, right? because std::ranges::less is not a class template anymore?

The good news is that literally everything else in this patch is good; I downloaded the raw diff and went through it in git diff, so it wasn't until the very end that I got to the newly added files and found the lack of tests for std::ranges::less. :)

I recommend splitting out the trivial whitespace diffs and just landing them for now.
(This will also help in case you end up having to revert the functional change later.)

libcxx/test/support/compare_types.h

8–9

Throughout: s/TEST_SUPPORT_COMPARISON_TYPES_H/TEST_SUPPORT_COMPARE_TYPES_H/

[zoecarver]

• Fix final (?) two nits.

[• Quuxplusone]

Seems OK to me at this point. Might want to have @ldionne take a look at it, though.
(I'm particularly wondering if there's a good way to de-duplicate the two copies of do_pointer_comparison_test.)

libcxx/include/functional

 
 POLICY:  For non-variadic implementations, the number of arguments is limited
          to 3.  It is hoped that the need for non-variadic implementations
          will be minimal.
 
 */
 
 #include <__config>
+#include <concepts>
 #include <type_traits>
 #include <typeinfo>
 #include <exception>
 #include <memory>
 #include <tuple>
 #include <utility>
 #include <version>
 
 #include <__functional_base>
struct identity {
     {
         return _VSTD::forward<_Tp>(__t);
     }
 
     using is_transparent = void;
 };
 #endif // _LIBCPP_STD_VER > 17
 
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
+namespace ranges {
+
+struct equal_to {
+  template <class _Tp, class _Up>
+  requires equality_comparable_with<_Tp, _Up> constexpr bool
+  operator()(_Tp &&__t, _Up &&__u) const {
+    return forward<_Tp>(__t) == forward<_Up>(__u);
+  }
+
+  using is_transparent = char;
+};
+
+struct not_equal_to {
+  template <class _Tp, class _Up>
+  requires equality_comparable_with<_Tp, _Up> constexpr bool
+  operator()(_Tp &&__t, _Up &&__u) const {
+    return !(forward<_Tp>(__t) == forward<_Up>(__u));
+  }
+
+  using is_transparent = char;
+};
+
+struct greater {
+  template <class _Tp, class _Up>
+  requires totally_ordered_with<_Tp, _Up> constexpr bool
+  operator()(_Tp &&__t, _Up &&__u) const {
+    return forward<_Up>(__u) < forward<_Tp>(__t);
+  }
+
+  using is_transparent = char;
+};
+
+struct less {
+  template <class _Tp, class _Up>
+  requires totally_ordered_with<_Tp, _Up> constexpr bool
+  operator()(_Tp &&__t, _Up &&__u) const {
+    return forward<_Tp>(__t) < forward<_Up>(__u);
+  }
+
+  using is_transparent = char;
+};
+
+struct greater_equal {
+  template <class _Tp, class _Up>
+  requires totally_ordered_with<_Tp, _Up> constexpr bool
+  operator()(_Tp &&__t, _Up &&__u) const {
+    return !(forward<_Tp>(__t) < forward<_Up>(__u));
+  }
+
+  using is_transparent = char;
+};
+
+struct less_equal {
+  template <class _Tp, class _Up>
+  requires totally_ordered_with<_Tp, _Up> constexpr bool
+  operator()(_Tp &&__t, _Up &&__u) const {
+    return !(forward<_Up>(__u) < forward<_Tp>(__t));
+  }
+
+  using is_transparent = char;
+};
+
+} // namespace ranges
+
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
+
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_FUNCTIONAL

libcxx/test/std/concepts/concepts.compare/concept.equalitycomparable/equality_comparable.compile.pass.cpp

 #include <memory>
 #include <mutex>
 #include <optional>
 #include <set>
 #include <unordered_map>
 #include <unordered_set>
 #include <vector>
 
-#include "../types.h"
+#include "compare_types.h"
 
 namespace fundamentals {
 static_assert(std::equality_comparable<int>);
 static_assert(std::equality_comparable<double>);
 static_assert(std::equality_comparable<void*>);
 static_assert(std::equality_comparable<char*>);
 static_assert(std::equality_comparable<char const*>);
 static_assert(std::equality_comparable<char volatile*>);

libcxx/test/std/concepts/concepts.compare/concept.equalitycomparable/equality_comparable_with.compile.pass.cpp

 #include <memory>
 #include <mutex>
 #include <optional>
 #include <set>
 #include <unordered_map>
 #include <unordered_set>
 #include <vector>
 
-#include "../types.h"
+#include "compare_types.h"
 
 template <class T, class U>
 constexpr bool check_equality_comparable_with() {
   constexpr bool result = std::equality_comparable_with<T, U>;
   static_assert(std::equality_comparable_with<U, T> == result);
   static_assert(std::equality_comparable_with<T, U const> == result);
   static_assert(std::equality_comparable_with<T const, U const> == result);
   static_assert(std::equality_comparable_with<T, U const&> == result);

libcxx/test/std/concepts/concepts.compare/concepts.totallyordered/totally_ordered.pass.cpp

 #include <map>
 #include <memory>
 #include <optional>
 #include <set>
 #include <unordered_map>
 #include <unordered_set>
 #include <vector>
 
-#include "../types.h"
+#include "compare_types.h"
 #include "test_macros.h"
 
 // `models_totally_ordered` checks that `std::totally_ordered` subsumes
 // `std::equality_comparable`. This overload should *never* be called.
 template <std::equality_comparable T>
 constexpr bool models_totally_ordered() noexcept {
   return false;
 }

libcxx/test/std/concepts/concepts.compare/concepts.totallyordered/totally_ordered_with.pass.cpp

 #include <map>
 #include <memory>
 #include <optional>
 #include <set>
 #include <unordered_map>
 #include <unordered_set>
 #include <vector>
 
-#include "../types.h"
+#include "compare_types.h"
 #include "test_macros.h"
 
 template <class T, class U>
 constexpr bool check_totally_ordered_with() noexcept {
   constexpr bool result = std::totally_ordered_with<T, U>;
   static_assert(std::totally_ordered_with<U, T> == result);
   static_assert(std::totally_ordered_with<T, U const> == result);
   static_assert(std::totally_ordered_with<T const, U const> == result);

libcxx/test/std/utilities/function.objects/comparisons/pointer_comparison_test_helper.h

-#ifndef POINTER_COMPARISON_TEST_HELPER_H
-#define POINTER_COMPARISON_TEST_HELPER_H
-
-#include <vector>
-#include <memory>
-#include <cstdint>
-#include <cassert>
-
-#include "test_macros.h"
-
-template <class T, template<class> class CompareTemplate>
-void do_pointer_comparison_test() {
-    typedef CompareTemplate<T*> Compare;
-    typedef CompareTemplate<std::uintptr_t> UIntCompare;
-#if TEST_STD_VER > 11
-    typedef CompareTemplate<void> VoidCompare;
-#else
-    typedef Compare VoidCompare;
-#endif
-    std::vector<std::shared_ptr<T> > pointers;
-    const std::size_t test_size = 100;
-    for (size_t i=0; i < test_size; ++i)
-        pointers.push_back(std::shared_ptr<T>(new T()));
-    Compare comp;
-    UIntCompare ucomp;
-    VoidCompare vcomp;
-    for (size_t i=0; i < test_size; ++i) {
-        for (size_t j=0; j < test_size; ++j) {
-            T* lhs = pointers[i].get();
-            T* rhs = pointers[j].get();
-            std::uintptr_t lhs_uint = reinterpret_cast<std::uintptr_t>(lhs);
-            std::uintptr_t rhs_uint = reinterpret_cast<std::uintptr_t>(rhs);
-            assert(comp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));
-            assert(vcomp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));
-        }
-    }
-}
-
-#endif // POINTER_COMPARISON_TEST_HELPER_H

libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <functional>
+
+// ranges::equal_to
+
+#include <functional>
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+#include "compare_types.h"
+#include "MoveOnly.h"
+#include "pointer_comparison_test_helper.h"
+
+struct NotTotallyOrdered {
+  bool operator==(NotTotallyOrdered) { return true; }
+};
+
+static_assert(!std::is_invocable_v<std::ranges::equal_to, NotTotallyOrdered,
+                                   NotTotallyOrdered>);
+static_assert(std::is_invocable_v<std::ranges::equal_to, explicit_operators,
+                                  explicit_operators>);
+
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::equal_to::is_transparent) {}
+
+constexpr bool test() {
+  auto fn = std::ranges::equal_to();
+
+  assert(fn(MoveOnly(42), MoveOnly(42)));
+
+  ForwardingTestObject a;
+  ForwardingTestObject b;
+  assert(!fn(a, b));
+  assert(fn(std::move(a), std::move(b)));
+
+  assert(!fn(1, 2));
+  assert(!fn(2, 1));
+  assert(fn(2, 2));
+
+  return true;
+}
+
+int main(int, char**) {
+
+  test();
+  static_assert(test());
+
+  // test total ordering of int* for equal_to<int*> and equal_to<void>.
+  do_pointer_comparison_test<int, std::equal_to>();
+
+  return 0;
+}

libcxx/test/std/utilities/function.objects/range.cmp/greater.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <functional>
+
+// ranges::greater
+
+#include <functional>
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+#include "compare_types.h"
+#include "MoveOnly.h"
+#include "pointer_comparison_test_helper.h"
+
+struct NotTotallyOrdered {
+  bool operator<(NotTotallyOrdered) { return true; }
+};
+
+static_assert(!std::is_invocable_v<std::ranges::greater, NotTotallyOrdered,
+                                   NotTotallyOrdered>);
+static_assert(std::is_invocable_v<std::ranges::greater, explicit_operators,
+                                  explicit_operators>);
+
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::greater::is_transparent) {}
+
+constexpr bool test() {
+  auto fn = std::ranges::greater();
+
+  assert(fn(MoveOnly(42), MoveOnly(41)));
+
+  ForwardingTestObject a;
+  ForwardingTestObject b;
+  assert(!fn(a, b));
+  assert(fn(std::move(a), std::move(b)));
+
+  assert(!fn(2, 2));
+  assert(!fn(1, 2));
+  assert(fn(2, 1));
+
+  return true;
+}
+
+int main(int, char**) {
+
+  test();
+  static_assert(test());
+
+  // test total ordering of int* for greater<int*> and greater<void>.
+  do_pointer_comparison_test<int, std::greater>();
+
+  return 0;
+}

libcxx/test/std/utilities/function.objects/range.cmp/greater_equal.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <functional>
+
+// ranges::greater_equal
+
+#include <functional>
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+#include "compare_types.h"
+#include "MoveOnly.h"
+#include "pointer_comparison_test_helper.h"
+
+struct NotTotallyOrdered {
+  bool operator<(NotTotallyOrdered) { return true; }
+};
+
+static_assert(!std::is_invocable_v<std::ranges::greater_equal,
+                                   NotTotallyOrdered, NotTotallyOrdered>);
+static_assert(std::is_invocable_v<std::ranges::greater_equal,
+                                  explicit_operators, explicit_operators>);
+
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::greater_equal::is_transparent) {}
+
+constexpr bool test() {
+  auto fn = std::ranges::greater_equal();
+
+  assert(fn(MoveOnly(42), MoveOnly(42)));
+
+  // These are the opposite of other tests.
+  ForwardingTestObject a;
+  ForwardingTestObject b;
+  assert(fn(a, b));
+  assert(!fn(std::move(a), std::move(b)));
+
+  assert(fn(2, 2));
+  assert(fn(2, 1));
+  assert(!fn(1, 2));
+
+  return true;
+}
+
+int main(int, char**) {
+
+  test();
+  static_assert(test());
+
+  // test total ordering of int* for greater_equal<int*> and greater_equal<void>.
+  do_pointer_comparison_test<int, std::greater_equal>();
+
+  return 0;
+}

libcxx/test/std/utilities/function.objects/range.cmp/less.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <functional>
+
+// ranges::less
+
+#include <functional>
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+#include "compare_types.h"
+#include "MoveOnly.h"
+#include "pointer_comparison_test_helper.h"
+
+// TODO: find a type that has a builtin type that has operator< but is not totally ordered
+// to test LWG3530.
+
+struct NotTotallyOrdered {
+  bool operator<(NotTotallyOrdered) { return true; }
+};
+
+static_assert(!std::is_invocable_v<std::ranges::less, NotTotallyOrdered,
+                                   NotTotallyOrdered>);
+static_assert(std::is_invocable_v<std::ranges::less, explicit_operators,
+                                  explicit_operators>);
+
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::less::is_transparent) {}
+
+constexpr bool test() {
+  auto fn = std::ranges::less();
+
+  assert(fn(MoveOnly(41), MoveOnly(42)));
+
+  ForwardingTestObject a;
+  ForwardingTestObject b;
+  assert(!fn(a, b));
+  assert(fn(std::move(a), std::move(b)));
+
+  assert(fn(1, 2));
+  assert(!fn(2, 2));
+  assert(!fn(2, 1));
+
+  return true;
+}
+
+int main(int, char**) {
+
+  test();
+  static_assert(test());
+
+  // test total ordering of int* for less<int*> and less<void>.
+  do_pointer_comparison_test<int, std::less>();
+
+  return 0;
+}

libcxx/test/std/utilities/function.objects/range.cmp/less_equal.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <functional>
+
+// ranges::less_equal
+
+#include <functional>
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+#include "compare_types.h"
+#include "MoveOnly.h"
+#include "pointer_comparison_test_helper.h"
+
+struct NotTotallyOrdered {
+  bool operator<(NotTotallyOrdered) { return true; }
+};
+
+static_assert(!std::is_invocable_v<std::ranges::less_equal, NotTotallyOrdered,
+                                   NotTotallyOrdered>);
+static_assert(std::is_invocable_v<std::ranges::less_equal, explicit_operators,
+                                  explicit_operators>);
+
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::less_equal::is_transparent) {}
+
+constexpr bool test() {
+  auto fn = std::ranges::less_equal();
+
+  assert(fn(MoveOnly(41), MoveOnly(42)));
+
+  // These are the opposite of other tests.
+  ForwardingTestObject a;
+  ForwardingTestObject b;
+  assert(fn(a, b));
+  assert(!fn(std::move(a), std::move(b)));
+
+  assert(fn(1, 2));
+  assert(fn(2, 2));
+  assert(!fn(2, 1));
+
+  return true;
+}
+
+int main(int, char**) {
+
+  test();
+  static_assert(test());
+
+  // test total ordering of int* for less_equal<int*> and less_equal<void>.
+  do_pointer_comparison_test<int, std::less_equal>();
+
+  return 0;
+}

libcxx/test/std/utilities/function.objects/range.cmp/not_equal_to.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <functional>
+
+// ranges::not_equal_to
+
+#include <functional>
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+#include "compare_types.h"
+#include "MoveOnly.h"
+#include "pointer_comparison_test_helper.h"
+
+struct NotTotallyOrdered {
+  bool operator==(NotTotallyOrdered) { return true; }
+};
+
+static_assert(!std::is_invocable_v<std::ranges::not_equal_to, NotTotallyOrdered,
+                                   NotTotallyOrdered>);
+static_assert(std::is_invocable_v<std::ranges::not_equal_to, explicit_operators,
+                                  explicit_operators>);
+
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::not_equal_to::is_transparent) {}
+
+constexpr bool test() {
+  auto fn = std::ranges::not_equal_to();
+
+  assert(fn(MoveOnly(41), MoveOnly(42)));
+
+  // These are the opposite of other tests.
+  ForwardingTestObject a;
+  ForwardingTestObject b;
+  assert(fn(a, b));
+  assert(!fn(std::move(a), std::move(b)));
+
+  assert(fn(1, 2));
+  assert(!fn(2, 2));
+  assert(fn(2, 1));
+
+  return true;
+}
+
+int main(int, char**) {
+
+  test();
+  static_assert(test());
+
+  // test total ordering of int* for not_equal_to<int*> and not_equal_to<void>.
+  do_pointer_comparison_test<int, std::not_equal_to>();
+
+  return 0;
+}

libcxx/test/support/compare_types.h

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef TEST_STD_CONCEPTS_COMPARISON_TYPES_H
 #define TEST_STD_CONCEPTS_COMPARISON_TYPES_H
 
 #include <compare>
 #include <concepts>
 #include <type_traits>
 
 // `noexcept` specifiers deliberately imperfect since not all programmers bother to put the
 // specifiers on their overloads.
 
 struct equality_comparable_with_ec1;
 struct no_neq;
 
 struct cxx20_member_eq {
   bool operator==(cxx20_member_eq const&) const = default;
 };
 
 struct cxx20_friend_eq {
   friend bool operator==(cxx20_friend_eq const&,
                          cxx20_friend_eq const&) = default;
 };
 
 struct member_three_way_comparable {
   auto operator<=>(member_three_way_comparable const&) const = default;
 };
 
 struct friend_three_way_comparable {
   friend auto operator<=>(friend_three_way_comparable const&,
friend bool operator==(explicit_operators const&,
                          equality_comparable_with_ec1 const&) noexcept;
   friend bool operator==(equality_comparable_with_ec1 const&,
                          explicit_operators const&) noexcept;
   friend bool operator!=(explicit_operators const&,
                          equality_comparable_with_ec1 const&) noexcept;
   friend bool operator!=(equality_comparable_with_ec1 const&,
                          explicit_operators const&) noexcept;
 };
 
 struct different_return_types {
   bool operator==(different_return_types) const noexcept;
   char operator!=(different_return_types) const noexcept;
   short operator<(different_return_types) const noexcept;
   int operator>(different_return_types) const noexcept;
   long operator<=(different_return_types) const noexcept;
   long long operator>=(different_return_types) const noexcept;
 
   friend signed char operator==(explicit_operators, different_return_types);
struct returns_int_ptr {
   friend int* operator>(returns_int_ptr, totally_ordered_with_others);
   friend int* operator>(totally_ordered_with_others, returns_int_ptr);
   friend int* operator<=(returns_int_ptr, totally_ordered_with_others);
   friend int* operator<=(totally_ordered_with_others, returns_int_ptr);
   friend int* operator>=(returns_int_ptr, totally_ordered_with_others);
   friend int* operator>=(totally_ordered_with_others, returns_int_ptr);
 };
 
+struct ForwardingTestObject {
+  constexpr bool operator<(ForwardingTestObject&&) && { return true; }
+  constexpr bool operator<(const ForwardingTestObject&) const& { return false; }
+
+  constexpr bool operator==(ForwardingTestObject&&) && { return true; }
+  constexpr bool operator==(const ForwardingTestObject&) const& {
+    return false;
+  }
+
+  constexpr bool operator!=(ForwardingTestObject&&) && { return true; }
+  constexpr bool operator!=(const ForwardingTestObject&) const& {
+    return false;
+  }
+
+  constexpr bool operator<=(ForwardingTestObject&&) && { return true; }
+  constexpr bool operator<=(const ForwardingTestObject&) const& {
+    return false;
+  }
+
+  constexpr bool operator>(ForwardingTestObject&&) && { return true; }
+  constexpr bool operator>(const ForwardingTestObject&) const& { return false; }
+
+  constexpr bool operator>=(ForwardingTestObject&&) && { return true; }
+  constexpr bool operator>=(const ForwardingTestObject&) const& {
+    return false;
+  }
+};
+
 #endif // TEST_STD_CONCEPTS_COMPARISON_TYPES_H

libcxx/test/support/pointer_comparison_test_helper.h

+#ifndef POINTER_COMPARISON_TEST_HELPER_H
+#define POINTER_COMPARISON_TEST_HELPER_H
+
+#include <vector>
+#include <memory>
+#include <cstdint>
+#include <cassert>
+
+#include "test_macros.h"
+
+template <class T, template <class> class CompareTemplate>
+void do_pointer_comparison_test() {
+  typedef CompareTemplate<T*> Compare;
+  typedef CompareTemplate<std::uintptr_t> UIntCompare;
+#if TEST_STD_VER > 11
+  typedef CompareTemplate<void> VoidCompare;
+#else
+  typedef Compare VoidCompare;
+#endif
+  std::vector<std::shared_ptr<T> > pointers;
+  const std::size_t test_size = 100;
+  for (size_t i = 0; i < test_size; ++i)
+    pointers.push_back(std::shared_ptr<T>(new T()));
+  Compare comp;
+  UIntCompare ucomp;
+  VoidCompare vcomp;
+  for (size_t i = 0; i < test_size; ++i) {
+    for (size_t j = 0; j < test_size; ++j) {
+      T* lhs = pointers[i].get();
+      T* rhs = pointers[j].get();
+      std::uintptr_t lhs_uint = reinterpret_cast<std::uintptr_t>(lhs);
+      std::uintptr_t rhs_uint = reinterpret_cast<std::uintptr_t>(rhs);
+      assert(comp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));
+      assert(vcomp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));
+    }
+  }
+}
+
+#endif // POINTER_COMPARISON_TEST_HELPER_H