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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

LGTM overall, thanks for working on this :-)



================
Comment at: libcxx/include/functional:3232
+  template <class _Tp, class _Up>
+  requires equality_comparable_with<_Tp, _Up> constexpr bool
+  operator()(_Tp &&__t, _Up &&__u) const {
----------------
Please make these `[[nodiscard]]`.


================
Comment at: libcxx/include/functional:3237
+
+  using is_transparent = char;
+};
----------------
Don't we typically make these `void`? Having said that, I'm all for giving them all a different type.


================
Comment at: libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp:32
+// We don't know what this type is, just that is should exist.
+void test_is_transparent(std::ranges::equal_to::is_transparent) {}
+
----------------
I find `static_assert`ing a requires-expression to be a lot clearer when it comes to checking that a typename is present.


================
Comment at: libcxx/test/std/utilities/function.objects/range.cmp/not_equal_to.pass.cpp:22
+
+struct NotTotallyOrdered {
+  bool operator==(NotTotallyOrdered) { return true; }
----------------
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?


================
Comment at: libcxx/test/support/pointer_comparison_test_helper.h:27-28
+  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();
----------------
Do we need to have loops in tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429