[libcxx-commits] [PATCH] D103056: [libcxx][ranges] Add `ranges::transform_view`.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

This isn't a review, just some early comments while I'm waiting on a build. Will give this a lot more attention later on.



================
Comment at: libcxx/include/__ranges/transform_view.h:103-118
+template<class _View>
+struct __iterator_concept {
+  using type = conditional_t<
+    random_access_range<_View>,
+    random_access_iterator_tag,
+    conditional_t<
+      bidirectional_range<_View>,
----------------
Probably better to take my one from D101922. It's easier to read and doesn't result in a lot of unnecessary instantiations.


================
Comment at: libcxx/include/__ranges/transform_view.h:279
+  friend constexpr decltype(auto) iter_move(const __iterator& __i)
+    // TODO: this noexcept looks wrong: why do we care about func's noexceptness?
+    noexcept(noexcept(_VSTD::invoke(*__i.__parent->__func, *__i.__current)))
----------------
`iter_move` should be noexcept, just like all move constructors. It also looks correct to me.


================
Comment at: libcxx/include/__ranges/transform_view.h:282
+  {
+    // TODO: this looks wrong: shouldn't this be the oposite.
+    if constexpr (is_lvalue_reference_v<decltype(*__i)>)
----------------
If `*__i` is already an rvalue, then we don't need to move it.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/range.transform.view.pass.cpp:70-75
+struct ForwardRange {
+  ForwardIter begin() const;
+  ForwardIter end() const;
+  ForwardIter begin();
+  ForwardIter end();
+};
----------------
See `test/support/test_range.h`


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/range.transform.view.pass.cpp:115
+
+struct SentinelView : std::ranges::view_base {
+  int count;
----------------
`InputView` is a sentinel view?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056