[libcxx-commits] [PATCH] D119619: [libc++][ranges] Implement `std::sortable`.

[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.

LGTM except for the tests. Thanks for working on this — you're very close to completing all the iterator concepts! :)



================
Comment at: libcxx/include/__iterator/sortable.h:30-31
+concept sortable =
+    permutable<_Iter> &&
+    indirect_strict_weak_order<_Comp, projected<_Iter, _Proj>>;
+
----------------
As usual, I'd prefer 2-space indents, even though clang-format disagrees.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.sortable/sortable.compile.pass.cpp:15
+
+#include <iterator>
+
----------------
`#include <functional>` for `ranges::less` (which I guess isn't used right now, but probably should be)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.sortable/sortable.compile.pass.cpp:33
+static_assert(!std::indirect_strict_weak_order<CompInt, NoIndirectStrictWeakOrder>);
+static_assert(!std::sortable<NoIndirectStrictWeakOrder>);
----------------
It occurs to me that you're missing tests for when `_Comp` is not `ranges::less` and/or `_Proj` is not `std::identity`.
Then, looking closer at this test, all sorts of problems pop into view.
```
static_assert( std::indirect_strict_weak_order<CompInt, AllConstraintsSatisfied>);
static_assert( std::sortable<AllConstraintsSatisfied>);
```
Maybe you meant either-or-both of these instead?
```
static_assert( std::indirect_strict_weak_order<ranges::less, AllConstraintsSatisfied>);
static_assert( std::sortable<AllConstraintsSatisfied>);
static_assert( std::indirect_strict_weak_order<CompInt, AllConstraintsSatisfied>);
static_assert( std::sortable<AllConstraintsSatisfied, CompInt>);
```

And then on line 32,
```
static_assert(!std::indirect_strict_weak_order<CompInt, NoIndirectStrictWeakOrder>);
```
is true, but has nothing to do with why `!std::sortable<NoIndirectStrictWeakOrder>`.

So, some more work is needed on this test.
This might also cause retroactive changes to `mergeable.compile.pass.cpp`, assuming it's missing the same kinds of coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119619