[libcxx-commits] [PATCH] D100429: [libc++][ranges] Add range.cmp: equal_to, not_equal_to, less, etc.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/utilities/function.objects/range.cmp/greater_equal.pass.cpp:33
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::greater_equal::is_transparent) {}
+
----------------
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.)


================
Comment at: libcxx/test/std/utilities/function.objects/range.cmp/greater_equal.pass.cpp:40
+
+  // These are the opposite of other tests.
+  ForwardingTestObject a;
----------------
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).


================
Comment at: libcxx/test/std/utilities/function.objects/range.cmp/not_equal_to.pass.cpp:27
+  friend bool operator==(const NotEqualityComparable&, const NotEqualityComparable&);
+  friend constexpr bool operator!=(const NotEqualityComparable&, const NotEqualityComparable&) = delete;
+};
----------------
remove `constexpr` here


================
Comment at: libcxx/test/support/compare_types.h:26
 struct cxx20_friend_eq {
-  friend bool operator==(cxx20_friend_eq const&,
-                         cxx20_friend_eq const&) = default;
+  friend bool operator==(cxx20_friend_eq const&, cxx20_friend_eq const&) = default;
 };
----------------
zoecarver wrote:
> If all these changes are distracting, I can move and format this file in a separate nfc patch. 
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.


================
Comment at: libcxx/test/support/compare_types.h:47-48
+  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;
 };
----------------
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100429/new/

https://reviews.llvm.org/D100429