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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
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->();
+    }
+  }
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > 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.
> > I totally agree that cpp17_xxx is really confusing. The standard uses these names 
> > https://eel.is/c++draft/iterator.traits#2
> > When the standard says cpp17-iterator, it really means the classic iterator that model the classic Iterator named requirement, which has nothing to do with c++ 17.  I don’t usually debate about names and I am ok with names that make sense for most people. for me I found the name cpp17, probably just because I used to how the spec calls those things
> > 
> > Regarding SFINAE, IIUC, for non template member function of a class template, the function body is instantiated as soon as the class template is instantiated, and this could trigger hard error in your case even you never call those functions 
> Every major compiler disagrees with you: https://godbolt.org/z/TxxsWxE9o.
I think you are right here. I was thinking more about explicit instantiation. https://godbolt.org/z/T7n6986rz  but we shouldn't be doing that as most stl templates are not SFINAE friendly.

anyway, IIUC, the purpose of this class is, in C++20 mode, this class "reverses" a C++17 iterator, and results in a C++17 iterator. The reason to have this class is that the `reverse_iterator`'s SFINAE broke some iterators in C++20 mode.
Could you please elaborate what exactly is broken? This sounds either a bug in the standard or in our implementation of  `reverse_iterator`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864