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

[• Quuxplusone]

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.

[philnik]

libcxx/include/__ranges/access.h

160

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

133

Is anywhere checked that ranges::cbegin() actually returns a const iterator?

libcxx/test/std/ranges/range.access/end.pass.cpp

254

What is the reason for (())?

[• Quuxplusone]

libcxx/include/__ranges/access.h

160

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

133

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

254

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]

libcxx/include/__ranges/access.h

160

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

133

I think it would be good to test that.

[• Quuxplusone]

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

[philnik]

You forgot a few closing namespace comments in your commit.

[• Quuxplusone]

libcxx/include/__ranges/access.h

167

@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]

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

212

Is it intentional to have (a) instead of a in this check? Ditto elsewhere in this file for invocable checks.

[• Quuxplusone]

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

212

Yes; I think this was answered here https://reviews.llvm.org/D116199#inline-1110929

[jloser]

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

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

133

Agreed re checking return types.

212

Yep, makes sense - thanks.

[ldionne]

libcxx/include/__ranges/access.h

149

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

[• Quuxplusone]

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

[• Quuxplusone]

Rebase on main.

libcxx/include/__ranges/access.h

public:
     void operator()(auto&&) const = delete;
   };
 } // namespace ranges::__end
 
 namespace ranges::inline __cpo {
   inline constexpr auto end = __end::__fn{};
 } // namespace ranges::__cpo
 
 namespace ranges::__cbegin {
   struct __fn {
     template <class _Tp>
       requires invocable<decltype(ranges::begin), _Tp const&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __t) const
       noexcept(noexcept(ranges::begin(_VSTD::as_const(__t))))
     {
       return ranges::begin(_VSTD::as_const(__t));
     }
 
     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
       noexcept(noexcept(ranges::begin(static_cast<_Tp const&&>(__t))))
     {
       return ranges::begin(static_cast<_Tp const&&>(__t));
     }
   };
 } // namespace ranges::__cbegin
 
 namespace ranges::inline __cpo {
   inline constexpr auto cbegin = __cbegin::__fn{};
 } // namespace ranges::__cpo
 
 namespace ranges::__cend {
   struct __fn {
     template <class _Tp>
       requires invocable<decltype(ranges::end), _Tp const&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __t) const
       noexcept(noexcept(ranges::end(_VSTD::as_const(__t))))
     {
       return ranges::end(_VSTD::as_const(__t));
     }
 
     template <class _Tp>
-    requires is_rvalue_reference_v<_Tp> && invocable<decltype(ranges::end), _Tp const&&>
+      requires is_rvalue_reference_v<_Tp&&> && invocable<decltype(ranges::end), _Tp const&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::end(static_cast<_Tp const&&>(__t))))
     {
       return ranges::end(static_cast<_Tp const&&>(__t));
     }
   };
 } // namespace ranges::__cend
 
 namespace ranges::inline __cpo {
   inline constexpr auto cend = __cend::__fn{};
 } // namespace ranges::__cpo
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_ACCESS_H

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 // UNSUPPORTED: libcpp-has-no-incomplete-ranges
 
 // std::ranges::begin
+// std::ranges::cbegin
 
 #include <ranges>
 
 #include <cassert>
 #include "test_macros.h"
 #include "test_iterators.h"
 
-using RangeBeginT = decltype(std::ranges::begin)&;
-using RangeCBeginT = decltype(std::ranges::cbegin)&;
+using RangeBeginT = decltype(std::ranges::begin);
+using RangeCBeginT = decltype(std::ranges::cbegin);
 
 static int globalBuff[8];
 
 static_assert(!std::is_invocable_v<RangeBeginT, int (&&)[10]>);
 static_assert( std::is_invocable_v<RangeBeginT, int (&)[10]>);
 static_assert(!std::is_invocable_v<RangeBeginT, int (&&)[]>);
 static_assert( std::is_invocable_v<RangeBeginT, int (&)[]>);
 

 };
 template<>
 inline constexpr bool std::ranges::enable_borrowed_range<EnabledBorrowingBeginMember> = true;
 
 constexpr bool testBeginMember() {
   BeginMember a;
   assert(std::ranges::begin(a) == &a.x);
   assert(std::ranges::cbegin(a) == &a.x);
+  static_assert(!std::is_invocable_v<RangeBeginT, BeginMember&&>);
+  static_assert(!std::is_invocable_v<RangeCBeginT, BeginMember&&>);
 
   NonConstBeginMember b;
   assert(std::ranges::begin(b) == &b.x);
+  static_assert(!std::is_invocable_v<RangeCBeginT, NonConstBeginMember&>);
 
   EnabledBorrowingBeginMember c;
+  assert(std::ranges::begin(c) == &globalBuff[0]);
+  assert(std::ranges::cbegin(c) == &globalBuff[0]);
   assert(std::ranges::begin(std::move(c)) == &globalBuff[0]);
+  assert(std::ranges::cbegin(std::move(c)) == &globalBuff[0]);
 
   BeginMemberFunction d;
   assert(std::ranges::begin(d) == &d.x);
   assert(std::ranges::cbegin(d) == &d.x);
 
   EmptyPtrBeginMember e;
   assert(std::ranges::begin(e) == &e.x);
   assert(std::ranges::cbegin(e) == &e.x);
struct BeginFunctionReturnsPtrConvertible {
   struct iterator { operator int*() const; };
   friend iterator begin(BeginFunctionReturnsPtrConvertible const&);
 };
 static_assert(!std::is_invocable_v<RangeBeginT, BeginFunctionReturnsPtrConvertible const&>);
 
 constexpr bool testBeginFunction() {
   BeginFunction a{};
   const BeginFunction aa{};
-  static_assert(!std::invocable<decltype(std::ranges::begin), decltype((a))>);
-  assert(std::ranges::begin(aa) == &aa.x);
+  static_assert(!std::invocable<RangeBeginT, decltype((a))>);
   assert(std::ranges::cbegin(a) == &a.x);
+  assert(std::ranges::begin(aa) == &aa.x);
   assert(std::ranges::cbegin(aa) == &aa.x);
 
   BeginFunctionByValue b{};
   const BeginFunctionByValue bb{};
   assert(std::ranges::begin(b) == &globalBuff[1]);
-  assert(std::ranges::begin(bb) == &globalBuff[1]);
   assert(std::ranges::cbegin(b) == &globalBuff[1]);
+  assert(std::ranges::begin(bb) == &globalBuff[1]);
   assert(std::ranges::cbegin(bb) == &globalBuff[1]);
 
   BeginFunctionEnabledBorrowing c{};
   const BeginFunctionEnabledBorrowing cc{};
   assert(std::ranges::begin(std::move(c)) == &globalBuff[2]);
-  static_assert(!std::invocable<decltype(std::ranges::cbegin), decltype(std::move(c))>);
+  assert(std::ranges::cbegin(std::move(c)) == &globalBuff[2]);
   assert(std::ranges::begin(std::move(cc)) == &globalBuff[2]);
   assert(std::ranges::cbegin(std::move(cc)) == &globalBuff[2]);
 
   BeginFunctionReturnsEmptyPtr d{};
   const BeginFunctionReturnsEmptyPtr dd{};
-  static_assert(!std::invocable<decltype(std::ranges::begin), decltype((d))>);
-  assert(std::ranges::begin(dd) == &dd.x);
+  static_assert(!std::invocable<RangeBeginT, decltype((d))>);
   assert(std::ranges::cbegin(d) == &d.x);
+  assert(std::ranges::begin(dd) == &dd.x);
   assert(std::ranges::cbegin(dd) == &dd.x);
 
   BeginFunctionWithDataMember e{};
   const BeginFunctionWithDataMember ee{};
-  static_assert(!std::invocable<decltype(std::ranges::begin), decltype((e))>);
+  static_assert(!std::invocable<RangeBeginT, decltype((e))>);
   assert(std::ranges::begin(ee) == &ee.x);
   assert(std::ranges::cbegin(e) == &e.x);
   assert(std::ranges::cbegin(ee) == &ee.x);
 
   BeginFunctionWithPrivateBeginMember f{};
   const BeginFunctionWithPrivateBeginMember ff{};
-  static_assert(!std::invocable<decltype(std::ranges::begin), decltype((f))>);
-  assert(std::ranges::begin(ff) == &ff.y);
+  static_assert(!std::invocable<RangeBeginT, decltype((f))>);
   assert(std::ranges::cbegin(f) == &f.y);
+  assert(std::ranges::begin(ff) == &ff.y);
   assert(std::ranges::cbegin(ff) == &ff.y);
 
   return true;
 }
 
 
 ASSERT_NOEXCEPT(std::ranges::begin(std::declval<int (&)[10]>()));
 ASSERT_NOEXCEPT(std::ranges::cbegin(std::declval<int (&)[10]>()));

 static_assert(!noexcept(std::ranges::cbegin(ntmbrr)));
 
 struct BeginReturnsArrayRef {
     auto begin() const noexcept -> int(&)[10];
 } brar;
 static_assert(noexcept(std::ranges::begin(brar)));
 static_assert(noexcept(std::ranges::cbegin(brar)));
 
-
 int main(int, char**) {
   testArray();
   static_assert(testArray());
 
   testBeginMember();
   static_assert(testBeginMember());
 
   testBeginFunction();
   static_assert(testBeginFunction());
 
   return 0;
 }

libcxx/test/std/ranges/range.access/end.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 // UNSUPPORTED: libcpp-has-no-incomplete-ranges
 
 // std::ranges::end
+// std::ranges::cend
 
 #include <ranges>
 
 #include <cassert>
 #include "test_macros.h"
 #include "test_iterators.h"
 
-using RangeEndT = decltype(std::ranges::end)&;
-using RangeCEndT = decltype(std::ranges::cend)&;
+using RangeEndT = decltype(std::ranges::end);
+using RangeCEndT = decltype(std::ranges::cend);
 
 static int globalBuff[8];
 
 static_assert(!std::is_invocable_v<RangeEndT, int (&&)[]>);
 static_assert(!std::is_invocable_v<RangeEndT, int (&)[]>);
 static_assert(!std::is_invocable_v<RangeEndT, int (&&)[10]>);
 static_assert( std::is_invocable_v<RangeEndT, int (&)[10]>);
 

 
 constexpr bool testEndMember() {
   EndMember a;
   assert(std::ranges::end(a) == &a.x);
   assert(std::ranges::cend(a) == &a.x);
 
   NonConstEndMember b;
   assert(std::ranges::end(b) == &b.x);
+  static_assert(!std::is_invocable_v<RangeCEndT, decltype((b))>);
 
   EnabledBorrowingEndMember c;
   assert(std::ranges::end(std::move(c)) == &globalBuff[0]);
+  assert(std::ranges::cend(std::move(c)) == &globalBuff[0]);
 
   EndMemberFunction d;
   assert(std::ranges::end(d) == &d.x);
   assert(std::ranges::cend(d) == &d.x);
 
   EmptyPtrEndMember e;
   assert(std::ranges::end(e) == &e.x);
   assert(std::ranges::cend(e) == &e.x);
struct BeginMemberEndFunction {
   int x;
   constexpr const int *begin() const { return nullptr; }
   friend constexpr const int *end(BeginMemberEndFunction const& bf) { return &bf.x; }
 };
 
 constexpr bool testEndFunction() {
   const EndFunction a{};
   assert(std::ranges::end(a) == &a.x);
+  assert(std::ranges::cend(a) == &a.x);
   EndFunction aa{};
+  static_assert(!std::is_invocable_v<RangeEndT, decltype((aa))>);
   assert(std::ranges::cend(aa) == &aa.x);
 
   EndFunctionByValue b;
   assert(std::ranges::end(b) == &globalBuff[1]);
   assert(std::ranges::cend(b) == &globalBuff[1]);
 
   EndFunctionEnabledBorrowing c;
   assert(std::ranges::end(std::move(c)) == &globalBuff[2]);
+  assert(std::ranges::cend(std::move(c)) == &globalBuff[2]);
 
   const EndFunctionReturnsEmptyPtr d{};
   assert(std::ranges::end(d) == &d.x);
+  assert(std::ranges::cend(d) == &d.x);
   EndFunctionReturnsEmptyPtr dd{};
+  static_assert(!std::is_invocable_v<RangeEndT, decltype((dd))>);
   assert(std::ranges::cend(dd) == &dd.x);
 
   const EndFunctionWithDataMember e{};
   assert(std::ranges::end(e) == &e.x);
+  assert(std::ranges::cend(e) == &e.x);
   EndFunctionWithDataMember ee{};
+  static_assert(!std::is_invocable_v<RangeEndT, decltype((ee))>);
   assert(std::ranges::cend(ee) == &ee.x);
 
   const EndFunctionWithPrivateEndMember f{};
   assert(std::ranges::end(f) == &f.y);
+  assert(std::ranges::cend(f) == &f.y);
   EndFunctionWithPrivateEndMember ff{};
+  static_assert(!std::is_invocable_v<RangeEndT, decltype((ff))>);
   assert(std::ranges::cend(ff) == &ff.y);
 
   const BeginMemberEndFunction g{};
   assert(std::ranges::end(g) == &g.x);
+  assert(std::ranges::cend(g) == &g.x);
   BeginMemberEndFunction gg{};
+  static_assert(!std::is_invocable_v<RangeEndT, decltype((gg))>);
   assert(std::ranges::cend(gg) == &gg.x);
 
   return true;
 }
 
 
 ASSERT_NOEXCEPT(std::ranges::end(std::declval<int (&)[10]>()));
 ASSERT_NOEXCEPT(std::ranges::cend(std::declval<int (&)[10]>()));