[libcxx-commits] [PATCH] D97176: [libcxx] adds concepts std::equality_comparable[_with]

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/concepts/comparison/types.h:50
+  friend bool operator!=(equality_comparable_with_ec1 const&,
+                         explicit_operators const&) noexcept;
+};
----------------
Quuxplusone wrote:
> Utter nit: I don't think most production code would bother to mark `operator==` as `noexcept`, so unless some test is relying on this, it might be better to remove the `noexcept` (for realism and to save characters). Or put it on some and not others (but with a comment explaining that that's on purpose).
> ```
>     friend int operator==(explicit_operators, eq_neq_different_return_types);
> ```
> is already missing the `noexcept`, but without the comment explaining that it was on purpose. ;)
Maybe I'm in some niche for putting `noexcept` everywhere possible (I am apparently for top-level `const`). I've added a comment on line 14.


================
Comment at: libcxx/test/std/concepts/comparison/types.h:85
+
+struct no_neq {
+  friend bool operator==(no_neq, no_neq) noexcept;
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > I'd say `deleted_ne`. This case is (intentionally) non-parallel to the `no_eq` case above.
> > I intended them to mirror one another, so I'm not sure I understand what you're getting at.
> `no_neq` has both an `operator==` and an `operator!=`, so when you try to do `x != x` you get the user-provided `operator!=` — which happens to be deleted, so `x != x` is ill-formed.
> 
> `no_eq` has an `operator!=` and no `operator==`, so when you try to do `x == x`... well, okay, I had thought that you'd get a synthesized candidate (if I'm using that term right) formed by negating `x != x`. I see from Godbolt that that doesn't actually happen. So indeed, `x == x` also ends up ill-formed.
> 
> IOW I was assuming that [this asymmetry](https://godbolt.org/z/71Kfae) would materially affect your test results... but it seems that it doesn't really affect them as much as I thought. So nvm.
> 
Yeah, only `operator==` and `operator<=>` have magical powers 🤷


================
Comment at: libcxx/test/std/concepts/comparison/types.h:163
+  bool operator==(one_way_ne const&) const = default;
+  friend bool operator==(one_way_ne, explicit_operators);
+  friend bool operator!=(one_way_ne, explicit_operators) = delete;
----------------
cjdb wrote:
> Quuxplusone wrote:
> > Should this `==` say `!=`? At a glance, I have //no// idea what the overload set ends up looking like here. Which if that's the point, it needs a comment. ;)
> Hmm... good point. I had to stop and think about it for a moment myself, so I agree //some// documentation is necessary (probably a `static_assert` to prove my intentions and code match). The overload set should look like this:
> ```
> bool operator==(one_way_ne const&) const&;
> bool operator!=(one_way_ne const&) const&; // generated
> 
> bool operator==(one_way_ne, explicit_operators);
> bool operator!=(one_way_ne, explicit_operators) = delete;
> bool operator==(explicit_operators), one_way_ne); // generated
> ```
PTAL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97176