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

Tue Feb 8 01:20:14 PST 2022

var-const added inline comments.

================
Comment at: libcxx/include/__ranges/access.h:16
 #include <__iterator/readable_traits.h>
+#include <__iterator/reverse_iterator.h>
 #include <__ranges/enable_borrowed_range.h>
----------------
Quuxplusone wrote:
> 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`).
Done. (Note that in `__iterator/`, the header structure is `access.h`/`reverse_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;
+  };
----------------
Quuxplusone wrote:
> 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.)
Thanks for calling it out. For now, I'd prefer to leave this as a copy-paste from the Standard, but I'm interested to play around with whether the two versions are equivalent. I'll let you know what I find.

================
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.
+  {
----------------
Quuxplusone wrote:
> 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.
> You suggested on D118963 that `sizeof(_Tp) != 0` would be better, and I agree with you. Please make the substitution throughout.
Done.
> 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).
Yeah, I was trying to use the same ordering and created an extra clause for handling arrays specifically to satisfy `2.2` (which is otherwise identical to `2.5`). Removed it now since like you said it is already handled implicitly.

> I suspect you can dig into this and find a situation where your current overload set produces ambiguity.
Hmm, both `Mine[10]` and `std::string[10]` compile fine. When I was writing this, I thought there would be no ambiguity because constraints are only checked on functions with the same signature; thus, template argument deduction would always prefer the `(_Tp (&)[_Np])` signature over the `(_Tp&&)` signature, before concepts come into play. I might well be wrong, though.

================
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)))
----------------
Quuxplusone wrote:
> 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.
This is a great catch, thanks! Added a test structure (`MemberBeginAndRBegin`) and made sure it fails without the code change.

================
Comment at: libcxx/include/__ranges/access.h:284
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
+    noexcept(noexcept(end(__t)))
+  {
----------------
Quuxplusone wrote:
> 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.)
(Yeah, I did that due to the recent patch/direction to remove `_VSTD`. Happy to use `_VSTD` if necessary)

================
Comment at: libcxx/include/__ranges/access.h:289
+
+  void operator()(auto&&) const = delete;
+};
----------------
Quuxplusone wrote:
> You don't need this — do you? Check whether `ranges::begin` has it, and be consistent with whatever it is that they do.
It is the same in `ranges::begin`. I don't know the rationale -- presumably if overload resolution selects a deleted function, it produces a less verbose error than when it cannot find a match.

================
Comment at: libcxx/include/__ranges/access.h:297
+} // namespace __cpo
+
+} // namespace ranges
----------------
Quuxplusone wrote:
> Remove blank line here (and presumably likewise throughout); try to be consistent with the existing code.
Removed. I don't want to block this patch on this, but in general I prefer to have blank lines. I feel like the opening (or closing) of a namespace and the first (last) element of that namespace don't have any special relationship to warrant grouping them together.

================
Comment at: libcxx/test/std/ranges/range.access/begin.pass.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
Quuxplusone wrote:
> 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!
Done: https://reviews.llvm.org/D119214.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119057