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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__iterator/sortable.h:30-31
+concept sortable =
+    permutable<_Iter> &&
+    indirect_strict_weak_order<_Comp, projected<_Iter, _Proj>>;
+
----------------
Quuxplusone wrote:
> As usual, I'd prefer 2-space indents, even though clang-format disagrees.
Thinking more about this, I can buy an argument in favor of a 2-space indent for concepts. The general spirit is to use a narrower indent for continuation of constructs that are expected to normally be multiline (e.g. a function body) and a wider, more noticeable indent for constructs that are normally a single line but had to be split into multiple lines. For concepts, clearly the former is more applicable than the latter. Thus, treating the `&&` and `||` more like `;` in a function seems reasonable in this case.


================
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>);
----------------
Quuxplusone wrote:
> 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.
Should be fixed, PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119619