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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % nits!



================
Comment at: libcxx/docs/Status/RangesPaper.csv:69
 | mergeable
 | sortable",[iterator.concepts],Unassigned,Not started
 `[std.iterator.tags] <https://wg21.link/std.iterator.tags>`_,"| `contiguous_iterator_tag <https://reviews.llvm.org/rG45d048c20440989df2b4e1be1f9343225e7741ab>`_
----------------
Could mark this "In Progress" now.


================
Comment at: libcxx/include/__iterator/permutable.h:27-29
+    forward_iterator<_Iterator> &&
+    indirectly_movable_storable<_Iterator, _Iterator> &&
+    indirectly_swappable<_Iterator, _Iterator>;
----------------
Perhaps 2-space indent?
Personally I'd use `_Ip` for the template parameter name, but whatever.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.compile.pass.cpp:38
+using NotIMS = forward_iterator<NonCopyable*>;
+void iter_swap(const NotIMS&, const NotIMS&);
+
----------------
Instead of customizing `iter_swap` for `forward_iterator`, I'd prefer to see a hidden-friend `swap` for `NonCopyable`. That would still satisfy all your requirements for this test, right?


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119222