The big change here is that they now work as intended for rvalues, e.g. ranges::cbegin(std::string_view("hello")).
Also, add tests verifying their return types.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__ranges/access.h | ||
---|---|---|
169–170 | Nit, pre-existing: I would swap is_rvalue_reference_v and invocable to align the invocalbes of this operator() and the above. | |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
155 | Is anywhere checked that ranges::cbegin() actually returns a const iterator? | |
libcxx/test/std/ranges/range.access/end.pass.cpp | ||
278 | What is the reason for (())? |
libcxx/include/__ranges/access.h | ||
---|---|---|
169–170 | 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. :) $ 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 | |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
155 | Nope. We never check the return type of either ranges::begin or ranges::cbegin (ditto end/cend). | |
libcxx/test/std/ranges/range.access/end.pass.cpp | ||
278 | 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&. |
libcxx/include/__ranges/access.h | ||
---|---|---|
169–170 | 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. | |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
155 | I think it would be good to test that. |
Landed the NFC whitespace changes separately in 3042091168e99323217223a959df8675bdfa9b3c, and then rebased on that.
libcxx/include/__ranges/access.h | ||
---|---|---|
177 | @philnik wrote:
If you're talking about e.g. here (missing the // namespace __cbegin), that's intentional — or at least consistent throughout. Everywhere we close one of these nested namespaces after the }; of a struct __fn, we omit the closing comment. I could have added closing comments throughout (which would have been consistent with the global rule), but I actually think it looks marginally better without them, so that was a sufficient excuse not to mess with it. If you're talking about anywhere else, that was unintentional. |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
---|---|---|
234 | Is it intentional to have (a) instead of a in this check? Ditto elsewhere in this file for invocable checks. |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
---|---|---|
234 | Yes; I think this was answered here https://reviews.llvm.org/D116199#inline-1110929 |
libcxx/include/__ranges/access.h | ||
---|---|---|
160 | I actually don't understand why we specify our implementation this way at all. If we look at http://eel.is/c++draft/range.access#cbegin, I would expect this instead: struct __fn { template <class _Tp> requires is_lvalue_reference_v<_Tp> [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const noexcept(noexcept(ranges::begin(static_cast<_Tp const&>(__t))) -> decltype(ranges::begin(static_cast<_Tp const&>(__t))) { return ranges::begin(static_cast<_Tp const&>(__t)); } template <class _Tp> [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const noexcept(noexcept(ranges::begin(static_cast<_Tp const&&>(__t))) -> decltype(ranges::begin(static_cast<_Tp const&&>(__t))) { return ranges::begin(static_cast<_Tp const&&>(__t)); } }; [untested but you get the point] Edit: It looks like you are doing that as part of D115607, and I think it would be fine to pull that part of the change into this review. You could rename it to something like "implement ranges::cbegin/ranges::cend per the spec, and you're adding a test for the one thing you know is being fixed concretely (in addition to making us closer to the spec). |
Jump straight to the "correct by design" version — this is scope creep, but I'm happy to do it!
Add return-type tests.
I actually don't understand why we specify our implementation this way at all. If we look at http://eel.is/c++draft/range.access#cbegin, I would expect this instead:
[untested but you get the point]
Edit: It looks like you are doing that as part of D115607, and I think it would be fine to pull that part of the change into this review. You could rename it to something like "implement ranges::cbegin/ranges::cend per the spec, and you're adding a test for the one thing you know is being fixed concretely (in addition to making us closer to the spec).