[libcxx-commits] [PATCH] D157193: [libc++][ranges] P2116R9: Implements `views::enumerate`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h:60
+template <class Iterator, class Sentinel>
+struct MinimalView : std::ranges::view_base {
+  constexpr explicit MinimalView(Iterator it, Sentinel sent) : it_(base(std::move(it))), sent_(base(std::move(sent))) {}
----------------
Nit: from the names, it's not obvious what is the difference between `Range` (which is also a view), `JustAView` and `MinimalView`. I understand that `Range` has some additional testing utilities (perhaps rename it to `TestView`? Not a great name, but perhaps just a little more descriptive). Can you explain a bit how `JustAView` and `MinimalView` differ in case it helps us come up with some more descriptive names?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h:74
+
+struct NotInvocable {};
+
----------------
`static_assert(!std::invocable<NotInvocable>);`?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h:98
+
+static_assert(std::ranges::range<NotAViewRange> && !std::ranges::view<NotAViewRange>);
+
----------------
Optional nit: in general, I'd suggest having one `static_assert` per check (so that should it fire, it's more obvious which exact check failed).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h:101
+template <bool IsNoexcept>
+class NoexceptIterMoveInputIterator {
+  int* it_;
----------------
Optional nit: the class name makes it sound like it's always `noexcept`. Perhaps something like `MaybeNoexceptIterMoveInputIterator`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157193