[libcxx-commits] [PATCH] D120180: [libc++][ranges] Implement changes to reverse_iterator from One Ranges Proposal.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

Here are some changes which I think don't affect the implementation, but please double-check my logic:

1. Removed the part that said

> "The fundamental relation between a reverse iterator and its corresponding iterator `i` is established by the identity: `&*(reverse_iterator(i)) == &*(i - 1)`.



2. Previously it was said that "`Iterator` shall meet the requirements of a `Cpp17RandomAccessIterator`" if certain operators are "referenced in a way that requires instantiation". Now it just says "instantiated".

3. Previously, `operator->` was described as "Returns: `addressof(operator*())`". Now the description says:

> Effects:
>
> - if `Iterator` is a pointer type, equivalent to: `return prev(current);`
> - otherwise, equivalent to: `return prev(current).operator->();`

It seems like the current implementation of `operator->` should satisfy this, but please let me know if it's wrong.



================
Comment at: libcxx/include/__iterator/iterator_traits.h:145
+// The `cpp17-*-iterator` exposition-only concepts are easily confused with the Cpp17*Iterator tables, so the
+// exposition-only concepts have been banished to a namespace that makes it obvious they have a niche use-case.
 namespace __iterator_traits_detail {
----------------
This is a drive-by change: I just found the original comment unclear.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:49
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+    static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>, "The Iterator "
+        "template argument must be a bidirectional iterator.");
----------------
http://eel.is/c++draft/reverse.iterators#reverse.iter.requirements-1:

> "The template parameter `Iterator` shall either meet the requirements of a `Cpp17BidirectionalIterator` (`[bidirectional.iterators]`) or model `bidirectional_­iterator` (`[iterator.concept.bidir]`)."

My understanding is that this is IFNDR, but adding a check seems like it could prevent harder-to-read errors during instantiation.

There's a similar requirement for random access iterators:

> "Additionally, `Iterator` shall either meet the requirements of a `Cpp17RandomAccessIterator` (`[random.access.iterators]`) or model `random_­access_­iterator` (`[iterator.concept.random.access]`) if the definitions of any of the members `operator+`, `operator-`, `operator+=`, `operator-=` (`[reverse.iter.nav]`), or `operator[]` (`[reverse.iter.elem]`), or the non-member operators (`[reverse.iter.cmp]`) `operator<`, `operator>`, `operator<=`, `operator>=`, `operator-`, or `operator+` (`[reverse.iter.nonmember]`) are instantiated (`[temp.inst]`)."

I'm not sure that can be done with a `static_assert` -- I tried placing it into the bodies of the mentioned operators, and the compiler produces errors trying to instantiate non-existing functions before it evaluates the `static_assert`.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:63
+#endif
+    using iterator_category = _If<__is_cpp17_random_access_iterator<_Iter>::value,
+                                  random_access_iterator_tag,
----------------
Note: this change to `iterator_category` was done previously for all language modes, even though it only applies to C++20 and later.


================
Comment at: libcxx/include/iterator:221
+    using iterator_concept  = see below; // since C++20
+    using iterator_category = typename iterator_traits<Iterator>::iterator_category; // since C++17, until C++20
+    using iterator_category = see below; // since C++20
----------------
Some of these changes might be a bit pedantic -- please let me know.


================
Comment at: libcxx/include/iterator:247
     constexpr reverse_iterator& operator-=(difference_type n);
-    constexpr reference         operator[](difference_type n) const;
+    constexpr unspecified       operator[](difference_type n) const;
+
----------------
>From what I could see, the return type was unspecified all the way back in C++03, so this looks like it was always incorrect.


================
Comment at: libcxx/include/iterator:253
+      friend constexpr void
+        iter_swap(const reverse_iterator& x,
+                  const reverse_iterator<Iterator2>& y) noexcept(see below);
----------------
The Standard's formatting is a bit peculiar here.


================
Comment at: libcxx/include/iterator:263
 constexpr bool                          // constexpr in C++17
-operator<(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+operator!=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
 
----------------
Use the same order as the Standard.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp:22
   static_assert(std::sentinel_for<I1, I1>);
-  static_assert(std::sentinel_for<I1, std::reverse_iterator<float*>>);
-  static_assert(!std::sized_sentinel_for<I1, std::reverse_iterator<float*>>);
----------------
These don't work anymore because there is no `operator==` to compare `I1::base() == std::reverse_iterator<float*>::base()`. Previously, trying to compare the two would fail upon instantiation, but this check didn't trigger instantiation, so it could succeed (though arguably it shouldn't have).

If you have ideas on how to replace these checks, please let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120180