This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement `ranges::{cbegin,cend}` per the spec.
ClosedPublic

Authored by Quuxplusone on Dec 22 2021, 7:44 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 22 2021, 7:44 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptDec 22 2021, 7:44 PM
philnik added inline comments.Dec 23 2021, 3:45 AM
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 (())?

Quuxplusone marked an inline comment as done.Dec 23 2021, 8:26 AM
Quuxplusone added inline comments.
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. :)
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
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).
(However, the return type of ranges::begin(R) does contribute to ranges::iterator_t<R>, for which we have a few tests.)

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&.
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.

Mordante added inline comments.Dec 23 2021, 9:21 AM
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.
Next time please don't mix code changes and whitespace fixes in one review.

libcxx/test/std/ranges/range.access/begin.pass.cpp
155

I think it would be good to test that.

Quuxplusone marked an inline comment as done.

Landed the NFC whitespace changes separately in 3042091168e99323217223a959df8675bdfa9b3c, and then rebased on that.

You forgot a few closing namespace comments in your commit.

libcxx/include/__ranges/access.h
177

@philnik wrote:

You forgot a few closing namespace comments in your commit.

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.

jloser added inline comments.Dec 26 2021, 12:35 PM
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

jloser accepted this revision.Jan 2 2022, 8:38 PM

LGTM % the comment about adding test for checking the return type.

libcxx/test/std/ranges/range.access/begin.pass.cpp
155

Agreed re checking return types.

234

Yep, makes sense - thanks.

ldionne requested changes to this revision.Jan 3 2022, 6:24 AM
ldionne added inline comments.
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).

This revision now requires changes to proceed.Jan 3 2022, 6:24 AM
Quuxplusone retitled this revision from [libc++] Fix ranges::{cbegin,cend} for rvalues. to [libc++] Implement `ranges::{cbegin,cend}` per the spec..
Quuxplusone edited the summary of this revision. (Show Details)

Jump straight to the "correct by design" version — this is scope creep, but I'm happy to do it!
Add return-type tests.

Rebase on main.

ldionne accepted this revision.Jan 4 2022, 12:51 PM
This revision is now accepted and ready to land.Jan 4 2022, 12:51 PM