[libcxx-commits] [PATCH] D129794: [libc++] Fix reverse_iterator::iterator_concept

[Hui via Phabricator via libcxx-commits]

huixie90 accepted this revision.
huixie90 added a comment.

LGTM with nits



================
Comment at: libcxx/test/support/test_iterators.h:227
+
+template <class It>
+class cpp20_random_access_iterator {
----------------
perhaps constrain `It` to be at least `random_access_iterator`?


================
Comment at: libcxx/test/support/test_iterators.h:235-240
+  using iterator_category = std::input_iterator_tag;
+  using iterator_concept  = std::random_access_iterator_tag;
+  using value_type        = typename std::iterator_traits<It>::value_type;
+  using difference_type   = typename std::iterator_traits<It>::difference_type;
+  using pointer           = It;
+  using reference         = typename std::iterator_traits<It>::reference;
----------------
This is more of FYI and you don't have to follow. To define new C++20 iterators with some level of c++17 backward compatibility support, we typically define:
- `iterator_category` if the class we define does model c++17 iteartor
- `iterator_concept`. this is obvious
- `value_type` for `indirectly_readable_traits`
- `difference_type` for `incrementable_traits`

We actually don't want to define `pointer` and `reference`. For C++20 concepts, they are not needed, they are deduced from `operator->` (if exists) and `operator*`. For C++17 backward compatibility, by not defining them, it will trigger the primary template `iteartor_traits` to properly generate them from `operator*` and `operator->` if they exist.

By defining them it will change the rule of `iterator_traits` primary template.

So the above is what we typically do when writing proposals for new range adaptors


================
Comment at: libcxx/test/support/test_iterators.h:248
+
+  template <class U, class = typename std::enable_if<std::is_default_constructible<U>::value>::type>
+  constexpr cpp20_random_access_iterator(cpp20_random_access_iterator<U>&& u) : it_(u.it_) {
----------------
if `U` itself already models `random_access_iterator`, it is already default constructible. If not, then perhaps some of the member functions will be broken. Do we need to check here?


================
Comment at: libcxx/test/support/test_iterators.h:253
+
+  constexpr reference operator*() const { return *it_; }
+  constexpr reference operator[](difference_type n) const { return it_[n]; }
----------------
Again you don't have to do. A really common category of iterators that model c++20 randam_access_iterator but only c++17 input_iterator are iterators with prvalue `reference` type, such as `iota`, `transform` iterator with `F` return by value. If I wrote this class, I'd simulate that. but you don't have to


================
Comment at: libcxx/test/support/test_iterators.h:309
+  }
+
+  friend constexpr It base(const cpp20_random_access_iterator& i) { return i.it_; }
----------------
This class defined `pointer` to be `It`, but not defining `operator->` , which is incorrect. I'd recommend just not define `pointer` since `operator->` isn't required


================
Comment at: libcxx/test/support/test_iterators.h:311
+  friend constexpr It base(const cpp20_random_access_iterator& i) { return i.it_; }
+
+  template <class T>
----------------
perhaps forward `iter_move` and `iter_swap` as well in case this class is used in the future by some other tests?


================
Comment at: libcxx/test/support/test_iterators.h:316
+
+static_assert(std::random_access_iterator<cpp20_random_access_iterator<int*>>);
+
----------------
perhaps also `static_assert` the `iterator_traits::iterator_category` is  not derived from random access tag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129794