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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a reviewer: ldionne.
Quuxplusone added a comment.

Somehow the size of `rend.pass.cpp` went //up// since I complained about its size. ;)
Anyway, once the "noexcept/expression-equivalent" thing is fixed, I think this is good to go.

@ldionne LGTY? Thoughts on cherry-picking into release/14.x so we can say we've got all of [range.access] in 14.x?



================
Comment at: libcxx/include/__ranges/rend.h:87
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
+    noexcept(noexcept(end(__t)))
+  {
----------------
var-const wrote:
> 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.
https://eel.is/c++draft/range.access.rbegin#2.5 is 100% clear that `ranges::rbegin(t)` must be expression-equivalent to `std::make_reverse_iterator(ranges::end(t))`, which means that the two expressions must have exactly the same noexceptness. If that means that right now today libc++'s `ranges::rbegin` is never-noexcept, then OK, that's what it means.

And yep,
https://eel.is/c++draft/reverse.iter.nonmember#7
is also clear that it remains conforming, even in C++20, for `make_reverse_iterator` to be never-noexcept.

The only thing we could do here that would be non-conforming, would be to have `make_reverse_iterator` non-noexcept and `ranges::rbegin` noexcept. That's definitely //not// permitted by the Standard, because it says the two must be expression-equivalent.

So, please do the "you must write it three times" thing for now (i.e. `noexcept(noexcept(the-exact-expression-you're-going-to-return))`). And please also triple-check the rest of your noexcept-specifications to make sure they're all correct by this logic.


================
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&>);
+
----------------
var-const wrote:
> 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.
Yeah, I'd prefer to see them removed. The place for the silent-SFINAE test is `end.pass.cpp`/`begin.pass.cpp`, and I'm 99% sure we've got those tests over there.


================
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&>);
+
----------------
var-const wrote:
> 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.
> It does add to the test coverage, but it's also more verbose and, IMO, doesn't reflect the intention quite as well

Well, maybe it //changes// the perceived intention. Instead of a bunch of similar tests — "this invocation compiles, this one doesn't compile..." — it gives us a bunch of similar tests plus a few "outlier" cases — "this invocation gives T, this invocation gives U, this one doesn't even compile, this invocation gives V..."
Anyway, we can defer this to a followup (or whatever).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119057