[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:337
+template <class _Iter>
+class _AlgRevIter {
+  _Iter __iter_;
----------------
Mordante wrote:
> Mordante wrote:
> > philnik wrote:
> > > philnik wrote:
> > > > Mordante wrote:
> > > > > Mordante wrote:
> > > > > > I'm not really happy with this name, it doesn't tell me anything. I also would like some comment what this iterator wrapper is.
> > > > > How is this class tested?
> > > > Do you have any naming suggestion? Or would you just like to have it spelled out more like `_AlgorihtmReverseIterator`?
> > > It's tested indirectly through `copy_backward`, `inplace_merge` and `stable_sort`. I'm planning to use it in more algorithms. Would you like have some explicit tests?
> > > Do you have any naming suggestion? Or would you just like to have it spelled out more like `_AlgorihtmReverseIterator`?
> > 
> > How do you about `cpp17_reverse_iterator`? I'm not sure whether we want to use `algorithm` in the name, this implies it should only be used in algorithms and not at other places.
> > 
> > At least with the comment it's easier to understand what the class is about. I had other associations with `Alg` which didn't make much sense.
> > It's tested indirectly through `copy_backward`, `inplace_merge` and `stable_sort`. I'm planning to use it in more algorithms. Would you like have some explicit tests?
> 
> I think it would be good to have some sanity checks for this class. That way when another test fails you can be sure it's the new code and not a bug in this class.
I think the implication you get from the name is correct. IMO we should only use `_AlgRevIter` (or whatever we will call it) in the places in which it is necessary. The only places I know of where that applies is in the classic STL algorithms. I think `__cpp17_reverse_iterator` sounds a bit too much like "this should be used pre-C++20 if you want a reverse_iterator", but it might be the better option if there are indeed other places where we want to use this reverse iterator.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:337
+template <class _Iter>
+class _AlgRevIter {
+  _Iter __iter_;
----------------
philnik wrote:
> Mordante wrote:
> > Mordante wrote:
> > > philnik wrote:
> > > > philnik wrote:
> > > > > Mordante wrote:
> > > > > > Mordante wrote:
> > > > > > > I'm not really happy with this name, it doesn't tell me anything. I also would like some comment what this iterator wrapper is.
> > > > > > How is this class tested?
> > > > > Do you have any naming suggestion? Or would you just like to have it spelled out more like `_AlgorihtmReverseIterator`?
> > > > It's tested indirectly through `copy_backward`, `inplace_merge` and `stable_sort`. I'm planning to use it in more algorithms. Would you like have some explicit tests?
> > > > Do you have any naming suggestion? Or would you just like to have it spelled out more like `_AlgorihtmReverseIterator`?
> > > 
> > > How do you about `cpp17_reverse_iterator`? I'm not sure whether we want to use `algorithm` in the name, this implies it should only be used in algorithms and not at other places.
> > > 
> > > At least with the comment it's easier to understand what the class is about. I had other associations with `Alg` which didn't make much sense.
> > > It's tested indirectly through `copy_backward`, `inplace_merge` and `stable_sort`. I'm planning to use it in more algorithms. Would you like have some explicit tests?
> > 
> > I think it would be good to have some sanity checks for this class. That way when another test fails you can be sure it's the new code and not a bug in this class.
> I think the implication you get from the name is correct. IMO we should only use `_AlgRevIter` (or whatever we will call it) in the places in which it is necessary. The only places I know of where that applies is in the classic STL algorithms. I think `__cpp17_reverse_iterator` sounds a bit too much like "this should be used pre-C++20 if you want a reverse_iterator", but it might be the better option if there are indeed other places where we want to use this reverse iterator.
I've copy-pasted the tests for `reverse_iterator` and removed a few tests (for `iter_move`, `iter_swap`, concepts and a few thing I haven't implemented like `reverse_iterator<const char*> = reverse_oterator<char*>` which should never be used in any algorithm anyways).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864