[libcxx-commits] [PATCH] D128864: [libc++] Fix algorihtms which use reverse_iterator

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked an inline comment as done.
philnik added inline comments.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:339
+template <class _Iter>
+class _AlgRevIter {
+  _Iter __iter_;
----------------
huixie90 wrote:
> question: is this class only supposed to be used in the classic algorithms? (that is why it doesn't need iter_move and iter_swap)
Yes, that's correct.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:349-350
+  using pointer = __iterator_pointer_type<_Iter>;
+  using iterator_concept =
+      _If<__is_cpp17_random_access_iterator<_Iter>::value, random_access_iterator_tag, bidirectional_iterator_tag>;
+  using value_type = iter_value_t<_Iter>;
----------------
huixie90 wrote:
> why c++20 `iterator_concept ` is defined in terms of c++17 trait?
This is how it's defined in `reverse_iterator`. (Although it looks like it's a bug there, but that's another PR)

Edit: I think it's actually safe to remove the `iterator_concept` entirely here. We shouldn't rely on it anywhere in the classic algorithms. We might want to optimize on it at some point, but hopefully at that point this code doesn't exist anymore.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:365-371
+  _LIBCPP_HIDE_FROM_ABI constexpr pointer operator->() const {
+    if constexpr (is_pointer_v<_Iter>) {
+      return std::prev(__iter_);
+    } else {
+      return std::prev(__iter_).operator->();
+    }
+  }
----------------
huixie90 wrote:
> could you please clarify why SFIANE is removed and how do you prevent error on instantiating the class template with an `_Iter` that does not support `->`? (assuming all non-template member functions are generated when class template is instantiated)
The new SFINAE in C++20 is the whole reason this PR exists at all. That's the part that breaks the classic algorithms. Regarding `operator->`: that has to exist for the classic algorithms. See https://en.cppreference.com/w/cpp/named_req/InputIterator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864