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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__ranges/rbegin.h:28
+
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > This should be guarded by `HAS_NO_INCOMPLETE_RANGES` too (and other headers too).
> Yes (perhaps), but that's D118736; I think as long as this header is consistent with what we //currently// do, it's better not to introduce potential churn over it here.
> 
> My impression of D118736 is that we're still kinda confused about what we want `HAS_NO_INCOMPLETE_RANGES` to encompass — I believe in my original PR it would have been included because it's in `ranges::`; and then in my subsequent tentative proposal it wouldn't have been because it has no ABI implications (no struct layout); and then in your current proposal it would again be included because it's in `ranges::` //and// no `std::` concept depends on it. I assume we'll talk about that offline soon. :)
My reasoning was pretty much what @Quuxplusone said -- until D118736 lands, I think new code should follow the existing approach. Do you think we should start applying `HAS_NO_INCOMPLETE_RANGES` now, without waiting for that patch?


================
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:
> 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.
Thanks for the explanation. Made this function, in both `rbegin` and `rend`, follow the "write it 3 times" approach (which also meant replacing the return types).

I think the other overloads are fine -- `noexcept` is exactly the expression being returned, and I think they can get away with returning `auto` because `_LIBCPP_AUTO_CAST` is applied to the return type (same as in the existing `begin` and `end`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119057