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

Zoe Carver via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu May 13 09:27:58 PDT 2021

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++() {
     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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list