[libcxx-commits] [PATCH] D100275: [libcxx][iterator][ranges] adds `forward_iterator` and `forward_range`

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
Please add two more tests: 
1. No `forward_iterator_tag`
2. Not incrementable. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp:27
+
+// TODO: add new `cxx20_*_iterator`s as they're added
+static_assert(!std::forward_iterator<cpp20_input_iterator<int*> >);
----------------
Do we still need the TODO here?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp:34
+static_assert(std::forward_iterator<int const volatile*>);
+
+struct not_input_iterator {
----------------
You don't have to take this comment, but maybe add a simple iterator that //is// a `forward_iterator` so that it's immediately obvious how the below types differ. Better yet, comment out the missing members. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp:44
+};
+static_assert(std::input_or_output_iterator<not_input_iterator> && !std::input_iterator<not_input_iterator> &&
+              !std::forward_iterator<not_input_iterator>);
----------------
Nit: Can you either give these all their own line or make them individual static asserts?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/subsumption.compile.pass.cpp:22
+template<std::input_iterator I>
+requires std::derived_from<std::_ITER_CONCEPT<I>, std::forward_iterator_tag>
+[[nodiscard]] constexpr bool check_subsumption() {
----------------
This either needs to be a `libcxx` only test or it needs to not use `_ITER_CONCEPT`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100275