[libcxx-commits] [PATCH] D106704: [libc++] Implement the output_iterator and output_range concepts

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

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

LGTM mod comments.



================
Comment at: libcxx/docs/Status/RangesPaper.csv:51
 | `random_access_iterator <https://llvm.org/D101316>`_
-| `contiguous_iterator <https://llvm.org/D101396>`_",iter_value_t,Christopher Di Bella,In progress
+| `contiguous_iterator <https://llvm.org/D101396>`_",iter_value_t,Various,✅
 `[indirectcallable.indirectinvocable] <http://wg21.link/indirectcallable.indirectinvocable>`_,"| indirectly_unary_invocable
----------------
Is `iter_value_t` in the "dependencies" column? I'd think that things that have been completed don't need to mention their dependencies anymore.
Honestly, things that have been completed don't need to mention their //authors// anymore; but I know this could be perceived as me complaining about other people getting internet points only because I myself have zero internet points at the moment. ;)


================
Comment at: libcxx/include/__iterator/concepts.h:133
+  requires (_Ip __it, _Tp&& __t) {
+    *__it++ = _VSTD::forward<_Tp>(__t); // not required to be equality-preserving
+  };
----------------
As usual, I think it would be nice to use `static_cast<_Tp&&>(__t)` here, but I don't object if we're not doing that (yet).


================
Comment at: libcxx/include/__ranges/concepts.h:96
   template <class _Tp>
   concept input_range = range<_Tp> && input_iterator<iterator_t<_Tp> >;
 
----------------
Might drive-by condense these `>>`s.


================
Comment at: libcxx/include/ranges:71
+  template<class R, class T>
+  concept output_range = see below;
+
----------------
+2 spaces here to match indentation below


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.output/output_iterator.compile.pass.cpp:50
+
+// Not satisfied when the iterator is not indirectly_writable with T
+struct NotIndirectlyWritable {
----------------
`// Not satisfied when we can't assign a T to the result of *it`
would be clearer as re parallel construction with line 39. It took me a while to figure out what was going on here.
Admittedly, that was because I fixated on the `void operator++(int)` on line 42. Consider making that something like `const T *operator++(int)` — i.e., you should work in the entire expression `*it++ = T()`, don't just make `*it++` ill-formed by itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106704