[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/include/functional:3233
+  requires equality_comparable_with<_Tp, _Up>
+  [[nodiscard]] constexpr bool operator()(_Tp &&__t, _Up &&__u) const {
+    return _VSTD::forward<_Tp>(__t) == _VSTD::forward<_Up>(__u);
----------------
Nit: `_Tp&& __t`, not `_Tp &&__t`, throughout.


================
Comment at: libcxx/include/functional:3244
+  [[nodiscard]] constexpr bool operator()(_Tp &&__t, _Up &&__u) const {
+    return !(_VSTD::forward<_Tp>(__t) == _VSTD::forward<_Up>(__u));
+  }
----------------
@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


================
Comment at: libcxx/include/functional:3254
+  [[nodiscard]] constexpr bool operator()(_Tp &&__t, _Up &&__u) const {
+    return _VSTD::forward<_Up>(__u) < _VSTD::forward<_Tp>(__t);
+  }
----------------
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.


================
Comment at: libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp:26-31
+  friend constexpr bool operator==(const NotEqualityComparable&,
+                                   const NotEqualityComparable&) {
+    return true;
+  }
+  friend constexpr bool operator!=(const NotEqualityComparable&,
+                                   const NotEqualityComparable&) = delete;
----------------
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.)


================
Comment at: libcxx/test/std/utilities/function.objects/range.cmp/equal_to.pass.cpp:36-37
+                                   NotEqualityComparable>);
+static_assert(std::is_invocable_v<std::ranges::equal_to, explicit_operators,
+                                  explicit_operators>);
+
----------------
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.


================
Comment at: libcxx/test/support/compare_types.h:8-9
 //===----------------------------------------------------------------------===//
 #ifndef TEST_STD_CONCEPTS_COMPARISON_TYPES_H
 #define TEST_STD_CONCEPTS_COMPARISON_TYPES_H
 
----------------
Probably a good idea to fix the include guard's name.


================
Comment at: libcxx/test/support/compare_types.h:578-580
+  constexpr bool operator==(const ForwardingTestObject&) const& {
+    return false;
+  }
----------------
Nit: please fix the whitespace to match lines 574-577, i.e., `{ return false; }` all on the same line. Ditto below.


================
Comment at: libcxx/test/support/pointer_comparison_test_helper.h:22-23
+  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;
----------------
zoecarver wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > cjdb wrote:
> > > > zoecarver wrote:
> > > > > cjdb wrote:
> > > > > > This can be made declarative using `std::generate_n`, and `std::make_shared<T>()` will make it clearer that there's no memory leaks.
> > > > > 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.
> > > > 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!
> > > +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.
> > 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.
> Updated this to be a static size array. Good call. No need for the template/shared_ptr. 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429