[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:
> > > > > 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`
If you want an example of what breaks you can look at `robust_against_cpp20_hostile_iterators.compile.pass.cpp`. Basically the problem is that the standard considers the `operator==` overloads ambiguous. Clang considers them not ambiguous and spits out a warning. The problem now is that overload resolution would be different if clang would also consider it not ambiguous in SFINAE contexts, so there it isn't ambiguous but an error. That means that overload resolution fails instead of selecting the correct overload.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864