[libcxx-commits] [PATCH] D105753: [libcxx][ranges] Add `ranges::common_view`.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:93-98
+constexpr auto operator-(sentinel_wrapper<RandomAccessIter> sent, RandomAccessIter iter) {
+  return sent.base().base() - iter.base();
+}
+constexpr auto operator-(RandomAccessIter iter, sentinel_wrapper<RandomAccessIter> sent) {
+  return iter.base() - sent.base().base();
+}
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > I'd prefer to see these as hidden friends of `RandomAccessIter`, or know the technical reason why they can't be. Ditto lines 72-77 above.
> RandomAccessIter is a type alias to `random_access_iterator`. I think we both agree we shouldn't add these as hidden friends to `random_access_iterator`.
Yes, although also it's not great to see them just hanging out in some random test file.
The next most //stylish// thing to do would be to make `SizedRandomcAccessView` [sic] have a nested type `struct sentinel`, and then make these hidden friends of `SizedRandomcAccessView::sentinel`. (I'm still rather unclear on why we have this generic "sentinel wrapper" type; it never seems to be quite what we want for testing.) But anyway, that's clearly too big a change for this PR, so I'm OK with leaving this hack for now.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:115
+    std::ranges::common_view<SizedForwardView> comm(SizedForwardView{buffer});
+    if (!std::is_constant_evaluated())
+      assert(*comm.begin() == 1);
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > Why do we need this `if`? What piece is not constexpr-friendly here?
> Basically none of common_iterator is constexpr. //Sigh//.
I see. Ugh. The constructor and accessors of `common_view` are constexpr, but `common_iterator` isn't. OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105753