[libcxx-commits] [PATCH] D102020: [libcxx][ranges] Add class ref_view.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/ref_view.h:35
+  class ref_view : public view_interface<ref_view<_Range>> {
+    _Range *__range = nullptr;
+
----------------
`__range_`


================
Comment at: libcxx/include/__ranges/ref_view.h:63
+
+    // TODO: This needs to use contiguous_range.
+    constexpr auto data() const
----------------
Has `contiguous_range` landed yet? If so, please DO this TODO; otherwise ok, not a blocker.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:66
+  struct sentinel {
+    friend constexpr bool operator==(sentinel, const cpp17_input_iterator<int*> iter) { return iter.base() == globalBuff + 8; }
+    friend constexpr std::ptrdiff_t operator-(sentinel, cpp17_input_iterator<int*>) { return -8; }
----------------
Pass-by-const-value alert! Remove `const`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:98-105
+template<class R>
+concept EmptyInvocable = requires (std::ranges::ref_view<R> view) { view.empty(); };
+
+template<class R>
+concept SizeInvocable = requires (std::ranges::ref_view<R> view) { view.size(); };
+
+template<class R>
----------------
Naming nit: `EmptyIsInvocable`, or `HasEmpty`; but let's avoid any potential confusion with the jargon use of `Invocable` as a noun. I'd go with `HasEmpty`, myself; it's shortest.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:124
+    Range range1;
+    std::ranges::ref_view<Range> view1 = range1 ;
+    assert(view1.begin() == globalBuff);
----------------
nit: whitespace


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:82-83
+
+bool operator==(const cpp20_input_iterator<int*> &lhs, int* rhs) { return lhs.base() == rhs; }
+bool operator==(int* lhs, const cpp20_input_iterator<int*> &rhs) { return rhs.base() == lhs; }
+
----------------
Quuxplusone wrote:
> Isn't this the sort of thing that the reversed synthesized candidates are supposed to handle for us?
> Orthogonally: hidden friend plz... or actually, wait, what? You're adding comparison operators to `cpp20_input_iterator` //itself?// That doesn't seem right. You should just make `Cpp20InputRange::end()` return a value of type `cpp20_input_iterator::sentinel`.
This comment relates to the code now on lines 77-78, and it hasn't been addressed yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102020