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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Continues to look pretty much good. Please see what you can do to shorten the tests — 535 lines for `rend.pass.cpp` seems excessive, for example. I know a big part of that comes from being consistent with the existing `end.pass.cpp`, though, so it's not a blocker.



================
Comment at: libcxx/include/__ranges/rbegin.h:18
+#include <__ranges/access.h>
+#include <__ranges/enable_borrowed_range.h>
+#include <__utility/auto_cast.h>
----------------
You don't need `enable_borrowed_range` in this file. (Just the `borrowed_range` concept itself, which comes from `concepts.h`). `enable_borrowed_range.h` is the bare-bones "just the trait" header for people who need to specialize the trait but don't care about querying the concept.

I suspect the similar inclusion in `__ranges/access.h` might also be unused.


================
Comment at: libcxx/include/__ranges/rbegin.h:20
+#include <__utility/auto_cast.h>
+#include <concepts>
+#include <type_traits>
----------------
Can you be more granular than all of `<concepts>`?


================
Comment at: libcxx/include/__ranges/rbegin.h:24
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
----------------



================
Comment at: libcxx/include/__ranges/rbegin.h:85
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
+    noexcept(noexcept(end(__t)))
+  {
----------------



================
Comment at: libcxx/include/__ranges/rend.h:17
+#include <__ranges/access.h>
+#include <__ranges/enable_borrowed_range.h>
+#include <__utility/auto_cast.h>
----------------
also `#include <__ranges/rbegin.h>`


================
Comment at: libcxx/include/__ranges/rend.h:87
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
+    noexcept(noexcept(end(__t)))
+  {
----------------
Consider adding a regression test, although maybe that'll end up looking too silly, once you try it.


================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:124-128
+struct EmptyRBeginMember {
+  struct iterator {};
+  iterator rbegin() const;
+};
+static_assert(!std::is_invocable_v<RangeRBeginT, EmptyRBeginMember const&>);
----------------
As in the other review, I'd say that `EmptyRBeginMember` is redundant with `RBeginMemberReturnsInt`; consider removing it.


================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:300
+
+
+struct MemberBeginEnd {
----------------
remove extra blank line


================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:364-365
+struct MemberBeginEndDifferentTypes {
+  bidirectional_iterator<int*> begin();
+  bidirectional_iterator<char*> end();
+};
----------------
This is an even simpler case, IIUC: both types are iterators, with the same value_type, interconvertible (well, at least in one direction), and yet //still// they aren't the same type, so the type is not rbegin'able.

I consider `FunctionBeginEndDifferentTypes` redundant with this test.


================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:382
+static_assert(!std::is_invocable_v<RangeRBeginT, MemberBeginEndForwardIterators&>);
+static_assert(!std::is_invocable_v<RangeCRBeginT, MemberBeginEndForwardIterators&>);
+
----------------
I consider `FunctionBeginEndForwardIterators` redundant with `MemberBeginEndForwardIterators`.


================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:401
+static_assert(!std::is_invocable_v<RangeRBeginT, FunctionBeginOnly&>);
+static_assert(!std::is_invocable_v<RangeCRBeginT, FunctionBeginOnly&>);
+
----------------
I consider `FunctionEndOnly` redundant with `MemberEndOnly` (I'd just keep the latter); and I'm not sure that `MemberBeginOnly` and `FunctionBeginOnly` are actually testing //anything// worth testing, since "obviously" one can't get a reverse-iterator-to-end-of-range out of nothing but a `begin()` method.


================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:301
+
+struct MemberBeginEnd {
+  int b, e;
----------------
var-const wrote:
> Note: these tests (up to and including `testBeginEnd`) are new (same in `rend.pass.cpp`).
SGTM. Do you have a test for `struct EndWithoutBegin { int *end() const; }` that proves such a type is //not// rbegin'able?


================
Comment at: libcxx/test/std/ranges/range.access/rend.pass.cpp:444
+static_assert( std::is_invocable_v<RangeREndT, MemberBeginAndRBegin&>);
+static_assert( std::is_invocable_v<RangeCREndT, MemberBeginAndRBegin&>);
+
----------------
LGTM, but please also test that
```
static_assert(std::same_as<std::invoke_result_t<RangeREndT, MemberBeginAndRBegin&>, int*>);
```
(and not `reverse_iterator<int*>`). Actually, it would be profitable to replace //all// your positive `std::is_invocable_v` assertions with positive `std::same_as` assertions. It's free test coverage!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119057