[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:
> > 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`?


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


================
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*() {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864