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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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;
----------------
huixie90 wrote:
> 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
Thanks for the explanation!


================
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_) {
----------------
huixie90 wrote:
> 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?
Nope, it's just a copy-paste.


================
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>
----------------
huixie90 wrote:
> perhaps forward `iter_move` and `iter_swap` as well in case this class is used in the future by some other tests?
I don't know if we will ever use this with move advanced iterators. I'd rather keep it at the minimum for `random_access_iterator`s for now. We can still update it later if we want.


================
Comment at: libcxx/test/support/test_iterators.h:316
+
+static_assert(std::random_access_iterator<cpp20_random_access_iterator<int*>>);
+
----------------
huixie90 wrote:
> perhaps also `static_assert` the `iterator_traits::iterator_category` is  not derived from random access tag?
I don't really see the value in that. You can just look at the typedef and know that it's the case. Verifying that `random_access_iterator` is satisfied is really hard OTOH.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129794