[libcxx-commits] [PATCH] D102037: [libcxx][views] Add drop_view.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Overall LGTM, left some comments and we just spoke offline too, so you know what else I want. Thanks a lot for working on this, I think this is a huge milestone!!



================
Comment at: libcxx/include/__ranges/drop_view.h:35
+  template<class _Range>
+  concept __simple_view =
+    view<_Range> && range<const _Range> &&
----------------
This should eventually be lifted to a common header because I believe many views are going to depend on this exposition only concept.


================
Comment at: libcxx/include/__ranges/drop_view.h:47
+public:
+    constexpr drop_view() noexcept = default;
+
----------------
The combination of `= default` on this constructor and default member initializers is IMO somewhat misleading. When I see `= default`, I generally think "oh, the constructor default constructs everything". But this is not the case here.

I think I would instead drop default-member-initializers and explicitly construct the members in the two constructors.

(Yes, I know default member initializers is the way the Standard exposts the class, but they don't have iterator caching).

__Edit:__

Actually coming back to this, I don't understand why we cache `begin(__base)` here in the constructor. Shouldn't we only cache it in `begin()`?


================
Comment at: libcxx/include/__ranges/drop_view.h:61
+      auto __tmp = ranges::begin(__base);
+      if constexpr (forward_range<_View>) {
+        if (__cached_begin != __tmp || __count == 0)
----------------
I think it would be more natural to use

```
if (!input_or_output_iterator<iterator_t<_View>>) {
 // cache here
}
```


================
Comment at: libcxx/include/__ranges/drop_view.h:64
+          return __cached_begin;
+      } else {}
+      ranges::advance(__tmp, __count, ranges::end(__base));
----------------
Is this intended? `else {}`

If not... there's something funky here if your tests are passing! If so, I would reorganize to avoid this unusual construct.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:89
+
+static_assert(std::same_as<decltype(ranges::drop_view(View(), 0)), ranges::drop_view<View>>);
+static_assert(std::same_as<decltype(ranges::drop_view(CopyableView(), 0)), ranges::drop_view<CopyableView>>);
----------------
Add a comment `// test CTAD`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037