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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/ref_view.h:39-42
+  template<class _Range>
+  concept sized_range = range<_Range> && requires(_Range& t) {
+    ranges::size(t);
+  };
----------------
Quuxplusone wrote:
> 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.
I posted this in another thread too, but I did a bad job documenting these concepts. They will both be implemented in their own patches at some point soon (I was hoping for today... but that didn't happen). 

I didn't realize they were needed until too late so I just copied them in; they're not part of this review.


================
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;
----------------
Quuxplusone wrote:
> Defaulted SMFs are constexpr (and noexcept) by default. You don't need this diff.
Fair enough, will remove. Semi-related: from [[ http://eel.is/c++draft/range.ref.view | the synopsis of ref_view ]]:
```
    constexpr ref_view() noexcept = default;
```


================
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_; }
 
----------------
Quuxplusone wrote:
> 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.
I'll do this in another PR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102020