[libcxx-commits] [PATCH] D116268: [libc++][ranges] Add indirectly_comparable concept

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

🎄😄



================
Comment at: libcxx/include/__iterator/iter_swap.h:102-103
 
+template <class _I1, class _I2, class _Rp, class _P1 = identity, class _P2 = identity>
+concept indirectly_comparable = indirect_binary_predicate<_Rp, projected<_I1, _P1>, projected<_I2, _P2>>;
+
----------------
This code looks fine, but why on earth is it in `iter_swap.h`?
It should be in `__iterator/concepts.h`, or (because I think that would create a cycle) just put it in `__iterator/indirectly_comparable.h`.

Somewhat tangential query: Is this the last of the `<iterator>` concepts? or are we still missing some?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/indirectcallable/indirectinvocable/indirectly_comparable.compile.pass.cpp:18-19
+
+#include "__iterator/readable_traits.h"
+#include "indirectly_readable.h"
+#include "test_iterators.h"
----------------
Direct inclusion of `<__iterator/readable_traits.h>` will fail the modules build.
I've never seen `libcxx/test/support/indirectly_readable.h` before but it looks crazy complicated; can we avoid using it here? I think all you actually need for this test would be something like
```
struct Deref {
    int operator()(int*) const;
};

static_assert(!std::indirectly_comparable<int, int, std::less<int>>);  // not dereferenceable
static_assert(!std::indirectly_comparable<int*, int*, int>);  // not a predicate
static_assert( std::indirectly_comparable<int*, int*, std::less<int>>);
static_assert(!std::indirectly_comparable<int**, int*, std::less<int>>);
static_assert( std::indirectly_comparable<int**, int*, std::less<int>, Deref>);
static_assert(!std::indirectly_comparable<int**, int*, std::less<int>, Deref, Deref>);
static_assert(!std::indirectly_comparable<int**, int*, std::less<int>, std::identity, Deref>);
static_assert( std::indirectly_comparable<int*, int**, std::less<int>, std::identity, Deref>);
```
And then a subsumption test in each direction:
```
template<class F> requires std::indirectly_comparable<int*, char*, F> && true
constexpr bool subsumes(F f) { return true; }

template<class F> requires std::indirect_binary_predicate<F, std::projected<int*, std::identity>, std::projected<char*, std::identity>>
void subsumes(F f);

template<class F> requires std::indirect_binary_predicate<F, std::projected<int*, std::identity>, std::projected<char*, std::identity>> && true
constexpr bool is_subsumed(F f) { return true; }

template<class F> requires std::indirectly_comparable<int*, char*, F>
void is_subsumed(F f);

static_assert(subsumes(std::less<int>()));
static_assert(is_subsumed(std::less<int>()));
```
https://godbolt.org/z/c9rEh8e8c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116268