[libcxx-commits] [PATCH] D119057: [libc++][ranges] Implement rbegin, rend, crbegin and crend.

Sat Feb 5 08:41:16 PST 2022

Quuxplusone added subscribers: ldionne, Quuxplusone.
Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Thanks for finishing up this clause!

I've reviewed the implementation but not yet the tests. Some significant code comments will lead to adding some extra tests. (And if you find that you want to add analogous regression tests to `begin.pass.cpp` and `end.pass.cpp` too, awesome!)

================
Comment at: libcxx/include/__ranges/access.h:16
 #include <__iterator/readable_traits.h>
+#include <__iterator/reverse_iterator.h>
 #include <__ranges/enable_borrowed_range.h>
----------------
Because of this new dependency (which is probably circular, once we get around to implementing C++20 `reverse_iterator`), and because the four functions you're adding are basically symmetric with the four in this file, please split them out into a new detail header. I suggest `rbegin.h` (for rbegin/crbegin) and `rend.h` (rend/crend) if you can get away with it. If that idea runs into problems for some reason, then I'd be reasonably happy with just `raccess.h` (by symmetry with `access.h`).

================
Comment at: libcxx/include/__ranges/access.h:224-227
+  requires(_Tp&& __t) {
+    { ranges::begin(__t) } -> same_as<decltype(ranges::end(__t))>;
+    { ranges::begin(__t) } -> bidirectional_iterator;
+  };
----------------
Note: This could be arguably more simply expressed as
```
template <class _Tp>
concept __can_reverse =
  __can_borrow<_Tp> &&
  same_as<iterator_t<_Tp>, sentinel_t<_Tp>> &&
  bidirectional_iterator<iterator_t<_Tp>>;
```
or even
```
template <class _Tp>
concept __can_reverse =
  __can_borrow<_Tp> &&
  common_range<_Tp> &&
  bidirectional_iterator<iterator_t<_Tp>>;
```
However, the way you wrote it is the closest to the Standard's actual wording (why they used that wording, I don't know) and perhaps marginally faster to compile, so, I think whatever you want to do here is fine.

(Alternatively, if you decide to dig into this and discover a way in which either of my rewrites is //not// equivalent to the Standard's wording, please make sure to add a regression test so that something will fail if we later decide to go with one of those rewrites anyway.)

================
Comment at: libcxx/include/__ranges/access.h:260
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[_Np]) const noexcept
+    requires (sizeof(*__t) != 0) // Disallow incomplete element types.
+  {
----------------
var-const wrote:
> "Otherwise, if `T` is an array type (`[term.array.type]`) and `remove_all_extents_t<T>` is an incomplete type, `ranges::rbegin(E)` is ill-formed with no diagnostic required."
You suggested on D118963 that `sizeof(_Tp) != 0` would be better, and I agree with you. Please make the substitution throughout.

Also, please order the overloads in the same order as the bullet points in the Standard, just for the benefit of future readers. http://eel.is/c++draft/range.access#rbegin-2  This is bullet point... uh... wait... actually this isn't one of the points at all!  The "bounded array of complete type" case is handled in the Standard by point (2.5).

I suspect you can dig into this and find a situation where your current overload set produces ambiguity. Something like this maybe?
```
struct Mine {
  friend Mine *rbegin(Mine *p) { return p; }
  friend Mine *rend(Mine *p) { return p; }
};
Mine a[10];
... std::ranges::rbegin(a) ...
```
If you find such a case (and I would bet money it's possible to find), then please add it as a regression test.

Edit to add: I wonder if `std::string[10]` is such a case! `std::string[10]` has an ADL `rbegin` and `rend` out of the box.

================
Comment at: libcxx/include/__ranges/access.h:283
+    requires __can_reverse<_Tp>
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
+    noexcept(noexcept(end(__t)))
----------------
var-const wrote:
> "Otherwise, if both ranges::begin(t) and ranges::end(t) are valid expressions of the same type which models bidirectional_iterator ([iterator.concept.bidir]), ranges::rbegin(E) is expression-equivalent to make_reverse_iterator(ranges::end(t))."
I believe your `__can_reverse` needs to include `!__unqualified_rbegin<_Tp> && !__member_rbegin<_Tp> &&`, or else you need to include them ad-hoc right here. I have a moderate preference for //not// ad-hoc, even if it means that you'll have to duplicate the concept into a `__can_reverse_begin` and a `__can_reverse_end` for use further down.
Please add a regression test that causes overload resolution ambiguity with your current overload set.

================
Comment at: libcxx/include/__ranges/access.h:284
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
+    noexcept(noexcept(end(__t)))
+  {
----------------
Attn @ldionne — it's our new policy to start using unadorned `std::` in new code, is that right? Or should we keep the `_VSTD::` convention in case the release/14.x process discovers an unexpected problem and we need to roll back the `#define _VSTD std` change?
(Using `std::` here is totally fine if it's our policy; I just want to hear a clear "yes" from @ldionne.)

================
Comment at: libcxx/include/__ranges/access.h:289
+
+  void operator()(auto&&) const = delete;
+};
----------------
You don't need this — do you? Check whether `ranges::begin` has it, and be consistent with whatever it is that they do.

================
Comment at: libcxx/include/__ranges/access.h:297
+} // namespace __cpo
+
+} // namespace ranges
----------------
Remove blank line here (and presumably likewise throughout); try to be consistent with the existing code.

================
Comment at: libcxx/include/__ranges/access.h:332
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[_Np]) const noexcept
+    requires (sizeof(*__t) != 0) // Disallow incomplete element types.
+  {
----------------
var-const wrote:
> "Otherwise, if `T` is an array type (`[term.array.type]`) and `remove_all_extents_t<T>` is an incomplete type, `ranges::rend(E)` is ill-formed with no diagnostic required."
As above: this overload as written is asking for //complete// types, which should be handled by points (2.4) and (2.5) instead; so remove this overload.

================
Comment at: libcxx/test/std/ranges/range.access/begin.pass.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
These changes to `begin.pass.cpp` and `end.pass.cpp` LGTM, but not directly relevant to this PR. I say go ahead and land these two files (assuming that they still pass after rebasing on trunk), and then this PR will get a bit shorter. Thanks for the cleanup!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119057