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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:337
+template <class _Iter>
+class _AlgRevIter {
+  _Iter __iter_;
----------------
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.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:337
+template <class _Iter>
+class _AlgRevIter {
+  _Iter __iter_;
----------------
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.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:357
+
+  _LIBCPP_HIDE_FROM_ABI constexpr auto base() const { return __iter_; }
+  _LIBCPP_HIDE_FROM_ABI constexpr decltype(auto) operator*() {
----------------
philnik wrote:
> Mordante wrote:
> > In general we prefer not to use compiler deduced `auto` on return types.
> That makes sense to me with ABI-relevant parts of the code, but what's wrong with it in an implementation-detail class?
In my experience it makes it harder to understand the code. Instead of looking at a function signature I need to look in the function to see the returned type. It just saves typing a few characters for the writer, but puts a higher burden on the reader. I agree with Google in that regard; code should be optimized for the reader.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864