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

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

Quuxplusone marked an inline comment as done.
Quuxplusone 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
----------------
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
```


================
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]);
 
----------------
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.)


================
Comment at: libcxx/test/std/ranges/range.access/end.pass.cpp:254
   EndFunction aa{};
+  static_assert(!std::is_invocable_v<RangeEndT, decltype((aa))>);
   assert(std::ranges::cend(aa) == &aa.x);
----------------
philnik wrote:
> What is the reason for `(())`?
Like `sizeof` and `alignas`, the `decltype` keyword also has two meanings. `decltype(id-expression)` means "the declared type of this variable"; `decltype(a-more-complex-expression)` means "the type //and value category// of this expression." So `decltype(aa)` is `EndFunction`, but `decltype((aa))` is `EndFunction&`.
Of course I could just have written `!std::is_invocable_v<RangeEndT, EndFunction&>`, but by line 283 the type names start getting unwieldy, so just using `decltype((xx))` consistently seems like the way to go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116199