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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/CMakeLists.txt:45
   __ranges/size.h
+  __ranges/ref_view.h
   __ranges/view_interface.h
----------------
Please alphabetize.


================
Comment at: libcxx/include/__ranges/ref_view.h:13-14
+#include <__config>
+#include <__iterator/iterator_traits.h>
+#include <__iterator/concepts.h>
+#include <__ranges/access.h>
----------------
Please alphabetize.


================
Comment at: libcxx/include/__ranges/ref_view.h:39-42
+  template<class _Range>
+  concept sized_range = range<_Range> && requires(_Range& t) {
+    ranges::size(t);
+  };
----------------
The `sized_range` concept should go in the same header that defines `ranges::size`.

I'm fairly confident that `concept borrowed_range` doesn't belong in `ref_view.h` either, but I have no particular suggestion of where it //should// go.  It's also a duplicate of `concept __can_borrow` from `__ranges/access.h`; please figure something out.


================
Comment at: libcxx/include/__ranges/ref_view.h:75
+    // TODO: This needs to use contiguous_range.
+    constexpr auto data() const requires contiguous_iterator<iterator_t<_Range>>
+    { return ranges::data(*__range); }
----------------
Nit: For readability, I'd like to see the `requires`-clauses on line 75 and 71 line-broken and indented the same way you've done on line 68.


================
Comment at: libcxx/include/__ranges/ref_view.h:84
+
+// clang-format off
+
----------------
Nit: I'd prefer to see all of these noisy `// clang-format {off,on}` comments eliminated. (I'm planning to do that once this code stabilizes, anyway. Just complainin'. It'd make all your PRs four lines shorter!)


================
Comment at: libcxx/include/ranges:86
 #include <__ranges/size.h>
+#include <__ranges/ref_view.h>
 #include <__ranges/view_interface.h>
----------------
Please alphabetize.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:21
+
+namespace ranges = std::ranges;
+
----------------
In @cjdb's tests he's been using `namespace stdr = std::ranges`, which has two advantages: shortness and greppability. I think you should make the switch. (My personal preference is `namespace rg`, but we've already got `stdr` in the codebase, and anyway it's easy to switch from anything to anything, as long as it's //not// `namespace ranges`.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:30-33
+  constexpr friend int* begin(Range const& range) { return globalBuff + range.start; }
+  constexpr friend int* end(Range const&) { return globalBuff + 8; }
+  constexpr friend int* begin(Range& range) { return globalBuff + range.start; }
+  constexpr friend int* end(Range&) { return globalBuff + 8; }
----------------
`friend constexpr`, not `constexpr friend` — but also, why are these friends instead of member `begin`/`end`? Making them members would be a lot less typing.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:43
+static_assert(!ValidRefView<BeginOnly>);
+static_assert(!ValidRefView<int (&)[4]>);
+
----------------
But `ValidRefView<int[4]>` would be OK, right?


================
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; }
+
----------------
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`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp:105
+    Range range;
+    ranges::ref_view<Range> view{range};
+    assert(view.begin() == globalBuff);
----------------
Throughout: I'd like to see these tests use `stdr::ref_view<Range> view = range;` instead of direct-curly-brace-initialization.



================
Comment at: libcxx/test/support/test_iterators.h:645-648
+  constexpr cpp20_input_iterator() = default;
 
-  cpp20_input_iterator(cpp20_input_iterator&&) = default;
-  cpp20_input_iterator& operator=(cpp20_input_iterator&&) = default;
+  constexpr cpp20_input_iterator(cpp20_input_iterator&&) = default;
+  constexpr cpp20_input_iterator& operator=(cpp20_input_iterator&&) = default;
----------------
Defaulted SMFs are constexpr (and noexcept) by default. You don't need this diff.


================
Comment at: libcxx/test/support/test_iterators.h:657-666
+  constexpr cpp20_input_iterator& operator++() {
     ++base_;
     return *this;
   }
 
-  void operator++(int) { ++base_; }
+  constexpr void operator++(int) { ++base_; }
 
----------------
This looks good. While you're in the vicinity, I recommend moving `cpp20_input_iterator` up next to the other test iterators (namely, next to `cpp17_input_iterator`), and making it look like them:
- remove the C++20 `iter_value_t` dependency, just use `iterator_traits` like they do
- remove the `[[nodiscard]]` cruft
- add the missing `base(I)` free function
- add the missing deleted `operator,`
- ifdef it to C++14-and-later since its definition doesn't depend on anything from '17 or '20 (except `iter_value_t` which we can remove)
- add a nested type `struct sentinel {}` which is comparable to `cpp20_input_iterator<T>` (via hidden friends)

Ping me if you want me to just do this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102020