[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:343
+public:
+  static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>);
+
----------------
Fix to only allow C++17 bidirectional iterators.


================
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:
> philnik wrote:
> > 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.
> Then this class is incorrectly constrained (the static assert bit) the input `_Iter` has to model the c++17 bidi iterator, because c++20 bidi iterator wouldn’t work. It may not have `->` , and it may have prvalue `reference`, which is not allowed in c++ 17 bidi
> 
> So the input `Iter` has to be c++17 and the result iterator is also c++17. Why not just call it c++17 reverse iterator. 
> 
> Regarding random access operations not sfinae , when you pass in c++17 bidi only , wouldn’t it error on instantiating those member functions?
Thanks, I made a note for myself at the correct place.

I think `_Cpp17RevIter` sounds too much like it should be use pre-C++20 if you want a reverse iterator, not that this is added for compatibility and should only be used in specific circumstances (specifically, that you get a user-defined iterator which may be C++20-hostile).

SFINAE only applies when you try to check if a function exists. If you never call the function the compiler doesn't instantiate it and you don't get an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864