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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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->();
+    }
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864