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

[Joe Loser via Phabricator via libcxx-commits]

jloser accepted this revision.
jloser added a comment.

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



================
Comment at: libcxx/test/std/ranges/range.access/begin.pass.cpp:212
   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);
----------------
Quuxplusone wrote:
> jloser wrote:
> > Is it intentional to have `(a)` instead of `a` in this check? Ditto elsewhere in this file for `invocable` checks.
> Yes; I think this was answered here https://reviews.llvm.org/D116199#inline-1110929
Yep, makes sense - thanks.


================
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]);
 
----------------
Mordante wrote:
> 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.
Agreed re checking return types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116199