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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Mar 3 17:35:07 PST 2021

Quuxplusone accepted this revision.
Quuxplusone added a comment.

Just some nits on tests. The concept definition looks like a straightforward transliteration of
so LGTM.

Comment at: libcxx/test/std/concepts/comparison/types.h:15
+struct boolean {
+  operator bool() const noexcept;
Would anything change if this operator were `explicit`? In production code I would expect their `operator bool` to be `explicit`, because all constructors and conversions should always be `explicit` (and for bool conversions in particular, the `explicit` doesn't normally get in the way, because contextual conversion to bool).

Comment at: libcxx/test/std/concepts/comparison/types.h:50
+  friend bool operator!=(equality_comparable_with_ec1 const&,
+                         explicit_operators const&) noexcept;
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. ;)

Comment at: libcxx/test/std/concepts/comparison/types.h:85
+struct no_neq {
+  friend bool operator==(no_neq, no_neq) noexcept;
I'd say `deleted_ne`. This case is (intentionally) non-parallel to the `no_eq` case above.

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;
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. ;)

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list