[libcxx-commits] [PATCH] D103056: [libcxx][ranges] Add `ranges::transform_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.

This is awesome work, thanks a lot!



================
Comment at: libcxx/include/__ranges/concepts.h:108
+  template<bool _Const, class _Tp>
+  using __maybe_const = conditional_t<_Const, const _Tp, _Tp>;
 } // namespace ranges
----------------
Is this used anywhere else other than in `transform_view`? If not, let's put it in `transform_view.h` only.


================
Comment at: libcxx/include/__ranges/transform_view.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
You should update the Ranges tracking paper too.


================
Comment at: libcxx/include/__ranges/transform_view.h:49-51
+  requires view<_View> && is_object_v<_Fn> &&
+           regular_invocable<_Fn&, range_reference_t<_View>> &&
+           __referenceable<invoke_result_t<_Fn&, range_reference_t<_View>>>
----------------
You could factor this out into a concept `__transform_view_constraints` and use that to simplify your declaration of the nested `iterator` type below? Just a suggestion, I find it kind of annoying to duplicate the whole 

```
requires view<_View> && is_object_v<_Fn> &&
           regular_invocable<_Fn&, range_reference_t<_View>> &&
           __referenceable<invoke_result_t<_Fn&, range_reference_t<_View>>>
```

spaghetti when defining the iterator.


================
Comment at: libcxx/include/__ranges/transform_view.h:102
+transform_view(_Range&&, _Fn)
+  -> transform_view<decltype(views::all(std::declval<_Range>())), _Fn>;
+
----------------
Please add a comment to replace by `all_t` so we don't forget.


================
Comment at: libcxx/include/__ranges/transform_view.h:178
+
+  constexpr decltype(auto) operator*() const { return _VSTD::invoke(*__parent_->__func_, *__current_); }
+
----------------
You can line break here?


================
Comment at: libcxx/include/__ranges/transform_view.h:178
+
+  constexpr decltype(auto) operator*() const { return _VSTD::invoke(*__parent_->__func_, *__current_); }
+
----------------
ldionne wrote:
> You can line break here?
Also, as a conforming extension, we can make this conditionally `noexcept`. (please add test)


================
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>,
----------------
zoecarver wrote:
> cjdb wrote:
> > Probably better to take my one from D101922. It's easier to read and doesn't result in a lot of unnecessary instantiations.
> Sorry, can you be more specific, where should I find the simpler `__iterator_concept` type?
I think Chris is talking about `__deduce_iterator_category`. But reading `__deduce_iterator_category`, I don't think it does what we want.

However, we could rewrite as:

```
template<class _Tp> struct __iterator_concept;
template<random_access_range _Tp> struct __iterator_concept<_Tp> { using type = random_access_iterator_tag; };
template<bidirectional_range _Tp> struct __iterator_concept<_Tp> { using type = bidirectional_iterator_tag; };
// etc...
```

However, I would rename it to `__transform_view_iterator_concept` or something along those lines because other views/iterators are going to have different mappings from the concept of the underlying view to the concept they themselves model.


================
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)))
----------------
cjdb wrote:
> `iter_move` should be noexcept, just like all move constructors. It also looks correct to me.
The `noexcept` looks right to me (as we just discussed live), however I think what we want instead is to add `noexcept(...)` as a conforming extension to `operator*`, and then make this one become `noexcept(noexcept(*__i))`.


================
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)>)
----------------
cjdb wrote:
> If `*__i` is already an rvalue, then we don't need to move it.
Actually, recording my discussion with Zoe: If `*__i` is a prvalue and we try to `std::move` from it, the return type will be deduced to `T&&`. So we're going to bind the prvalue to an rvalue reference, which will be dangling as soon as the function finishes. So the way it is written right now is definitely correct.


================
Comment at: libcxx/include/ranges:80
   template <class _Tp>
     concept common_range = see below;
 
----------------
Please update the synopsis to match the current state of implementation.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/base.pass.cpp:23
+  std::ranges::transform_view<View, PlusPlus> transformView1;
+  auto base1 = std::move(transformView1).base();
+  assert(std::ranges::begin(base1) == globalBuff);
----------------
Please don't use `auto` since we're trying to also validate the return type of the function.

Also, we should add a `static_assert(std::is_same<decltype(xxx.base()), ...>);`. This ensures that e.g. we're not returning a `View const&` from the `base()` function, which I could picture being a mistake.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/begin.pass.cpp:28-30
+  std::ranges::transform_view transformView1(View{buff}, PlusPlus{});
+  assert(transformView1.begin().base() == buff);
+  assert(*transformView1.begin() == 1);
----------------
Can you enclose those into scopes like

```
{
  std::ranges::transform_view transformView(View{buff}, PlusPlus{});
  assert(transformView.begin().base() == buff);
  assert(*transformView.begin() == 1);
}
```

etc...

That way, you don't even have to number the variables, and IMO that makes the test easier to read.


Applies here and elsewhere, as you see fit.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/star.pass.cpp:18
+int main(int, char**) {
+  // __iterator::operator* tested elsewhere.
+
----------------
Please add a specific test.

In particular, I'd like to see a test where the function object returns a `T`, a `T&` and a `T&&`, and a confirmation that it works properly. It's also a great place to test the extension of `noexcept` we'll apply to it.

The same comment applies to `operator[]` for testing with different ref-qualified types.

Basically, the idea is that if we somehow messed up the return type of `operator*`, we should have something that catches it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056