[libcxx-commits] [PATCH] D116199: [libc++] Fix ranges::{cbegin, cend} for rvalues.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__ranges/access.h:160
     template <class _Tp>
-    requires is_rvalue_reference_v<_Tp> && invocable<decltype(ranges::begin), _Tp const&&>
+      requires is_rvalue_reference_v<_Tp&&> && invocable<decltype(ranges::begin), _Tp const&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
----------------
Quuxplusone wrote:
> philnik wrote:
> > Nit, pre-existing: I would swap `is_rvalue_reference_v` and `invocable` to align the `invocalbe`s of this `operator()` and the above.
> On the one hand, sure, you may have seen in D115607 that I'm still aiming to completely rewrite this whole thing, so I'm not particularly attached to what's here now. :)
> On the other hand, `is_rvalue_reference_v` is much much cheaper to evaluate than `std::invocable`, so it's good that we get the short-circuiting here.
> ```
> $ cat x.cpp
> #include <ranges>
> template<int N> auto ptr_to_array() -> int(*)[N];
> #define X(N) (void)std::ranges::cbegin(*ptr_to_array<N>());
> #define X4(N) X(N) X(N+1) X(N+2) X(N+3)
> #define X16(N) X4(N) X4(N+4) X4(N+8) X4(N+12)
> #define X64(N) X16(N) X16(N+16) X16(N+32) X16(N+48)
> #define X256(N) X64(N) X64(N+64) X64(N+128) X64(N+192)
> #define X1024(N) X256(N) X256(N+256) X256(N+512) X256(N+768)
> static void test() { X1024(1) }
> 
> $ time bin/clang++ -std=c++20 x.cpp -c -DSWAP_ORDER=0
> real	0m2.982s
> user	0m2.788s
> sys	0m0.170s
> 
> $ time bin/clang++ -std=c++20 x.cpp -c -DSWAP_ORDER=1
> real	0m4.771s
> user	0m4.513s
> sys	0m0.236s
> ```
> ...and for the record, maybe D115607 needs to look at compile times too...
> ```
> $ git checkout D115607; ninja cxx; time bin/clang++ -std=c++20 x.cpp -c
> real	0m4.166s
> user	0m3.924s
> sys	0m0.206s
> ```
I don't mind whitespace changes per se, but I really dislike them in this patch. This kind of obscures the real changes in this file.
Next time please don't mix code changes and whitespace fixes in one review.


================
Comment at: libcxx/test/std/ranges/range.access/begin.pass.cpp:133
   assert(std::ranges::begin(std::move(c)) == &globalBuff[0]);
+  assert(std::ranges::cbegin(std::move(c)) == &globalBuff[0]);
 
----------------
Quuxplusone wrote:
> philnik wrote:
> > Is anywhere checked that `ranges::cbegin()` actually returns a const iterator?
> Nope. We never check the return //type// of either `ranges::begin` or `ranges::cbegin` (ditto `end`/`cend`).
> (However, the return type of `ranges::begin(R)` does contribute to `ranges::iterator_t<R>`, for which we have a few tests.)
I think it would be good to test that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116199