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

Tue Apr 20 15:46:23 PDT 2021

zoecarver added inline comments.

================
Comment at: libcxx/include/functional:3244
+  [[nodiscard]] constexpr bool operator()(_Tp &&__t, _Up &&__u) const {
+    return !(_VSTD::forward<_Tp>(__t) == _VSTD::forward<_Up>(__u));
+  }
----------------
tcanens wrote:
> zoecarver wrote:
> > zoecarver wrote:
> > > Quuxplusone wrote:
> > > > cjdb wrote:
> > > > > zoecarver wrote:
> > > > > > tcanens wrote:
> > > > > > > cjdb wrote:
> > > > > > > > tcanens wrote:
> > > > > > > > > Quuxplusone wrote:
> > > > > > > > > > tcanens wrote:
> > > > > > > > > > > cjdb wrote:
> > > > > > > > > > > > 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.
> > > > > > > > > > > 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.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > @tcanens: Uh-oh. And the resolution of https://cplusplus.github.io/LWG/issue3530 doesn't affect your statement?
> > > > > > > > > > Could you post an example test case where the difference between `!ranges::equal_to{}(FWD(t), FWD(u))` and `FWD(t) != FWD(u)` is observable?
> > > > > > > > > > And I would bet that the difference between `!ranges::equal_to{}(FWD(t), FWD(u))` and `!(FWD(t) == FWD(u))` is also observable, because the latter might be applying `!` to a type other than `bool`?
> > > > > > > > > ```
> > > > > > > > > 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.
> > > > > > > > > 
> > > > > > > > > > And I would bet that the difference between `!ranges::equal_to{}(FWD(t), FWD(u))` and `!(FWD(t) == FWD(u))` is also observable, because the latter might be applying `!` to a type other than bool?
> > > > > > > > > 
> > > > > > > > > //`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.
> > > > > > > > >  And the resolution of https://cplusplus.github.io/LWG/issue3530 doesn't affect your statement?
> > > > > > > > 
> > > > > > > > No, because that's exclusively to do with pointers.
> > > > > > > > 
> > > > > > > > > Could you post an example test case where the difference between `!ranges::equal_to{}(FWD(t), FWD(u))` and `FWD(t) != FWD(u)` is observable?
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > 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.
> > > > > > > > 
> > > > > > > > > And I would bet that the difference between `!ranges::equal_to{}(FWD(t), FWD(u))` and `!(FWD(t) == FWD(u))` is also observable, because the latter might be applying `!` to a type other than `bool`?
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > 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.
> > > > > > 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. 
> > > > > 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.
> > > > 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. :)
> > > > However, before landing, please do change it to use `!=` //locally// and verify that at least one of your test cases "successfully fails."  (IOW, I believe Tim when he says we can't use `!=`; but if it's true we can't use `!=`, then we should have a regression test for that. We shouldn't land this without some such "successfully failing" test.)
> > > 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. 
> > > 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.
> > 
> > @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?
> "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.
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. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429