[libcxx-commits] [PATCH] D119222: [libc++][ranges] Implement `permutable`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with comments.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.compile.pass.cpp:45-48
+// Note: it is impossible for an iterator to satisfy `indirectly_movable_storable` but not `indirectly_swappable`:
+// `indirectly_swappable` requires both iterators to be `indirectly_readable` and for `ranges::iter_swap` to be
+// well-formed for both iterators. `indirectly_movable_storable` also requires the iterator to be `indirectly_readable`.
+// `ranges::iter_swap` is always defined for `indirectly_movable_storable` iterators.
----------------
Quuxplusone wrote:
> I agree with your logic. Spelling it out here doesn't seem useful, though, because it's not the //form// of the argument that's difficult; it's whether all of the premises are (still) factually correct. The reader has to visit cppreference to find that out, which means they're not reading your comment after the first sentence anyway.
I personally dislike "cryptic" or "clever" comments that state something without explaining it at all. It assumes that the reader will immediately understand why the statement is true.

This comment saves me time because I can read and understand the reasoning without having to work it out myself.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.subsumption.compile.pass.cpp:19
+  requires std::indirectly_movable_storable<I, I>
+constexpr bool subsumes_indirectly_movable_storable() {
+  return false;
----------------
Quuxplusone wrote:
> var-const wrote:
> > Please let me know if these subsumption tests aren't applicable.
> Works for me.
> I would have done a single overload set, like
> ```
> template<class I> void test_subsumption() requires std::forward_iterator<I>;
> template<class I> void test_subsumption() requires std::indirectly_movable_storable<I, I>;
> template<class I> void test_subsumption() requires std::indirectly_swappable<I, I>;
> template<class I> constexpr bool test_subsumption() requires std::permutable<I> { return true; }
> static_assert(test_subsumption<int*>()); // unambiguous
> ```
> but I could believe that's too "clever." Whatever you want to do here is okay, AFAIC.
I think this is a shorter, more copy-pastable and generally simpler way to test for subsumption than what we seem to do elsewhere. I would encourage @var-const to use something like this.

I thought it out, and I *think* this is 100% equivalent to the tests we have right now. If I missed a difference, it might be worth pointing it out @Quuxplusone .


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.subsumption.compile.pass.cpp:36
+}
+
+template<class I>
----------------
I would add a test for subsumption of `forward_iterator`, like you're doing for the other ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119222