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

Tue Feb 8 21:42:54 PST 2022

var-const added inline comments.

================
Comment at: libcxx/include/__ranges/rbegin.h:18
+#include <__ranges/access.h>
+#include <__ranges/enable_borrowed_range.h>
+#include <__utility/auto_cast.h>
----------------
Quuxplusone wrote:
> 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.
Right. `access.h` still needs to keep the include (it uses `enable_borrowed_range` in the definition of `__can_borrow`).

================
Comment at: libcxx/include/__ranges/rbegin.h:20
+#include <__utility/auto_cast.h>
+#include <concepts>
+#include <type_traits>
----------------
Quuxplusone wrote:
> Can you be more granular than all of `<concepts>`?
Done (and it looks like `access.h` doesn't need `<concepts>` at all).

================
Comment at: libcxx/include/__ranges/rend.h:87
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
+    noexcept(noexcept(end(__t)))
+  {
----------------
Quuxplusone wrote:
> Consider adding a regression test, although maybe that'll end up looking too silly, once you try it.
Thanks for catching this! Added two tests structs (`NoThrowBeginThrowingEnd` and `NoThrowEndThrowingBegin`).

I think, however, that it has to be just `ranges::begin(__t)`, not `std::make_reverse_iterator(ranges::begin(__t))`. `make_reverse_iterator` is not marked `noexcept`, so IIUC, it will always lead to `noexcept(false)`. At the same time, it doesn't throw any exceptions of its own, thus the actual behavior solely depends on whether `ranges::begin(__t)` can throw. Does this sound right to you? I can confirm that adding `make_reverse_iterator` breaks the existing `noexcept` tests.

================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:300
+
+
+struct MemberBeginEnd {
----------------
Quuxplusone wrote:
> remove extra blank line
Hmm, this is consistently used in the existing `begin/end.pass.cpp` tests to separate different groups of test cases (basically, two empty lines after every test function). I don't particularly like the style, but I followed it for consistency. If these are to be removed, they should be removed from the existing tests as well. What do you think?

================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:364-365
+struct MemberBeginEndDifferentTypes {
+  bidirectional_iterator<int*> begin();
+  bidirectional_iterator<char*> end();
+};
----------------
Quuxplusone wrote:
> 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.
Replaced `char*` with `const int*`. I'd rather keep `bidirectional_iterator` for consistency with other test cases.

================
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&>);
+
----------------
Quuxplusone wrote:
> I consider `FunctionBeginEndForwardIterators` redundant with `MemberBeginEndForwardIterators`.
See my comment below re. `Function/MemberEndOnly`.

================
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&>);
+
----------------
Quuxplusone wrote:
> 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.
I don't feel particularly strongly about keeping these. My intention was to make sure that all these incorrect cases are silently SFINAE'd away as opposed to leading to compilation errors (and they come two versions, member and free, because these are handled by different overloads). So the idea is not to make sure that these can't work (which, like you said, is pretty obvious), but rather that they fail in the expected quiet fashion. If you still feel this isn't worth testing, I can remove these.

================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:301
+
+struct MemberBeginEnd {
+  int b, e;
----------------
Quuxplusone wrote:
> 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?
I think `MemberEndOnly` is the equivalent test.

================
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&>);
+
----------------
Quuxplusone wrote:
> 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!
Done.

> 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!

I'm not sure. It does add to the test coverage, but it's also more verbose and, IMO, doesn't reflect the intention quite as well. In any case, if this is to be done, it should also be done in the existing tests, so it seems more suitable for a separate patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119057