Adds the six new concept constrained comparisons.
Details
- Reviewers
cjdb ldionne • Quuxplusone EricWF - Group Reviewers
Restricted Project - Commits
- rG879cbac08ba1: [libc++][ranges] Add range.cmp: equal_to, not_equal_to, less, etc.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM overall, thanks for working on this :-)
libcxx/include/functional | ||
---|---|---|
3233 | Please make these [[nodiscard]]. | |
3238 | 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 | ||
33 | 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 | ||
23 | 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 | ||
28–29 | Do we need to have loops in tests? |
libcxx/test/std/utilities/function.objects/range.cmp/less.pass.cpp | ||
---|---|---|
24 | 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. | |
27 | Use hidden friends; they're harder to screw up. Here I assume leaving off the const was accidental, right? |
libcxx/include/functional | ||
---|---|---|
3288 | Use using is_transparent = void;, for consistency. |
libcxx/test/support/pointer_comparison_test_helper.h | ||
---|---|---|
28–29 | I'd rather not update these tests. Also, I don't think this (albeit nested) loop is adding too much time. |
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. | |
28–29 | 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'm confused. Didn't you just add this file? |
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. | |
28–29 |
No, I moved it from libcxx/test/std/utilities/function.objects/comparisons/pointer_comparison_test_helper.h.
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. |
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! | |
28–29 | Ah, it's stress testing, not correctness testing. Carry on then. |
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. |
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. |
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. |
It hasn't been officially accepted yet, so it's not in the status page. Otherwise, yes, you're right.
libcxx/include/functional | ||
---|---|---|
3234 | Nit: _Tp&& __t, not _Tp &&__t, throughout. | |
3245 | @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 | |
3255 | 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. |
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 | ||
7–9 | Probably a good idea to fix the include guard's name. | |
516–518 | 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.
but you aren't testing
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. |
libcxx/include/functional | ||
---|---|---|
3245 | 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 |
libcxx/test/support/compare_types.h | ||
---|---|---|
26 | If all these changes are distracting, I can move and format this file in a separate nfc patch. |
libcxx/include/functional | ||
---|---|---|
3244 | noexcept specifiers would be appreciated. | |
3245 | 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 |
Yes, this was intentional. |
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.) | |
libcxx/test/std/utilities/function.objects/range.cmp/not_equal_to.pass.cpp | ||
28 | remove constexpr here | |
libcxx/test/support/compare_types.h | ||
26 | FYI, no objection from me if you want to push a whitespace-only cleanup. (But I don't find these changes particularly distracting, either.) | |
47–48 | 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==. |
libcxx/test/support/compare_types.h | ||
---|---|---|
47–48 |
equality_comparable1. I didn't expect people would like equality_comparable_with_equality_comparable1. Perhaps control would be a more descriptive name.
Many types that predate C++20 have operator!=.
noexcept is fine, unless you can provide a technical reason why it "shouldn't" be there. |
libcxx/include/functional | ||
---|---|---|
3245 | 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. |
libcxx/include/functional | ||
---|---|---|
3245 | @tcanens: Uh-oh. And the resolution of https://cplusplus.github.io/LWG/issue3530 doesn't affect your statement? |
libcxx/include/functional | ||
---|---|---|
3245 |
No, because that's exclusively to do with pointers.
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.
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. |
libcxx/include/functional | ||
---|---|---|
3245 | 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.
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. |
libcxx/include/functional | ||
---|---|---|
3245 | 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. |
libcxx/include/functional | ||
---|---|---|
3245 | 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. |
LGTM pending feedback resolution.
libcxx/include/functional | ||
---|---|---|
3229–3230 | Perhaps we should put this in __detail/noexcept_helpers.h? | |
3245 | 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. |
libcxx/include/functional | ||
---|---|---|
3229–3230 | 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." | |
3233 | I like [[nodiscard]] here. But shouldn't we be expressing it via _LIBCPP_NODISCARD_EXT? 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. | |
3245 | 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. :) |
libcxx/include/functional | ||
---|---|---|
3229–3230 | @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.) | |
3245 | 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. |
libcxx/include/__detail/noexcept_helpers.h | ||
---|---|---|
26 | We never get a chance to #undef this now but I think that's OK. |
libcxx/include/functional | ||
---|---|---|
3245 |
@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? |
libcxx/include/functional | ||
---|---|---|
3245 | "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. |
libcxx/include/functional | ||
---|---|---|
3245 | 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. |
libcxx/include/functional | ||
---|---|---|
3245 |
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).
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. |
libcxx/include/functional | ||
---|---|---|
3245 | 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. |
libcxx/include/functional | ||
---|---|---|
3236 | The implicit conversion to bool is not captured in the noexcept, ditto elsewhere. | |
libcxx/test/support/pointer_comparison_test_helper.h | ||
12 | 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". |
libcxx/include/__detail/noexcept_helpers.h | ||
---|---|---|
26 | Yes, that part is okay. 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 | ||
12 | 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. |
libcxx/include/functional | ||
---|---|---|
510–511 | (Now moot, but just for your future information) libc++ sorts _ asciibetically prior to a: |
- Fix noxecpt (remove header)
- Re-write pointer_comparison_test_helper.h again.
libcxx/test/support/pointer_comparison_test_helper.h | ||
---|---|---|
12 | The loop is back. But in this case, think it both increases coverage and readability. Let me know what you think. |
libcxx/test/support/pointer_comparison_test_helper.h | ||
---|---|---|
5 | No need for std::vector or memory to be included here. |
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:
(&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].) |
Okay, the builds passed (after only 11 hours). @Quuxplusone how's this looking to you?
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. | |
libcxx/test/support/compare_types.h | ||
7–9 | Throughout: s/TEST_SUPPORT_COMPARISON_TYPES_H/TEST_SUPPORT_COMPARE_TYPES_H/ |
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.)
We never get a chance to #undef this now but I think that's OK.