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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__iterator/permutable.h:27-29
+    forward_iterator<_Iterator> &&
+    indirectly_movable_storable<_Iterator, _Iterator> &&
+    indirectly_swappable<_Iterator, _Iterator>;
----------------
Quuxplusone wrote:
> Perhaps 2-space indent?
> Personally I'd use `_Ip` for the template parameter name, but whatever.
The indent width for split lines doesn't seem to be specified in the LLVM style guide, but clang-format thinks it's `4`: https://github.com/llvm/llvm-project/blob/07486395d2d05c9c567994456774cafdcc1611d0/clang/lib/Format/Format.cpp#L1163
I think there is some convenience in having regular indentation differ from indentation for split lines. If you feel otherwise, let me know.


================
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&);
+
----------------
Quuxplusone wrote:
> 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?
Done. Yes, I think it's better this way.


================
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.
----------------
ldionne wrote:
> 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.
I'd prefer to keep the comment (I'm open to shortening the explanation if you have any specific suggestions). I think it would make it easier for the reader to verify my logic if they don't have to make any assumptions, and should also make cross-checking with cppreference faster.


================
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;
----------------
ldionne wrote:
> 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 .
Thanks! I like this a lot -- it's more concise and easier to copy and/or extend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119222