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

Christopher Di Bella via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri Apr 16 23:22:04 PDT 2021


cjdb requested changes to this revision.
cjdb added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/functional:3243
+  requires equality_comparable_with<_Tp, _Up>
+  [[nodiscard]] constexpr bool operator()(_Tp &&__t, _Up &&__u) const {
+    return !(_VSTD::forward<_Tp>(__t) == _VSTD::forward<_Up>(__u));
----------------
noexcept specifiers would be appreciated.


================
Comment at: libcxx/include/functional:3244
+  [[nodiscard]] constexpr bool operator()(_Tp &&__t, _Up &&__u) const {
+    return !(_VSTD::forward<_Tp>(__t) == _VSTD::forward<_Up>(__u));
+  }
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > @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
> 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. 
Per [[ http://eel.is/c++draft/range.cmp | 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.


================
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>);
+
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > 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.
> 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 
> but I think `explicit_operators` sort of indicates that the individual operators are declared.

Yes, this was intentional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429



More information about the libcxx-commits mailing list