[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 inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/transform_view.h:33
+
+// REVIEW-NOTE: This is *not* part of the transform_view patch and will be added by a *separate* PR.
+template<class _I1, class _I2 = _I1>
----------------
Reminder to fix this.


================
Comment at: libcxx/include/__ranges/transform_view.h:78
+  }
+  constexpr __iterator<true>  begin() const
+    noexcept(noexcept(ranges::begin(__base_)))
----------------
Stray whitespace!


================
Comment at: libcxx/include/__ranges/transform_view.h:86
+
+  constexpr __sentinel<false> end()
+    noexcept(noexcept(ranges::end(__base_)))
----------------
You added `noexcept` to several `begin()` and `end()` methods, but I don't think they are necessary for your tests anymore if you use `declval()`, as we discussed offline. For consistency, I wouldn't add those `noexcept` as conforming extensions.


================
Comment at: libcxx/include/optional:909
     const value_type&
-    operator*() const&
+    operator*() const& noexcept
     {
----------------
Those need a `LIBCPP_STATIC_ASSERT(noexcept(...))` test added somewhere in the test suite.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/arithmetic.pass.cpp:13
+
+// __iterator::operator{++,--,+=,-=}
+
----------------
This could say something like `transform_view::<iterator>::operatorXXX` instead to make it clear it's an exposition only type (applies elsewhere too).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/iter_move.pass.cpp:38
+    std::ranges::transform_view transformView2(View{buff}, PlusPlus{});
+    static_assert( noexcept(std::ranges::iter_move(transformView1.begin())));
+    static_assert(!noexcept(std::ranges::iter_move(transformView2.begin())));
----------------
You could use `std::declval<std::ranges::iterator_t<transform_view>&>()` instead of `.begin()` to avoid having to mark `.begin()` as `noexcept`, can't you?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/star.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Filename: `deref.pass.cpp` instead?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/types.pass.cpp:13
+
+// Iterator member typedefs.
+
----------------
Can you spell out which ones you're expecting?

It's useful when e.g. updating tests after those requirements change, if they do.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/types.h:127
+
+struct PlusPlus {
+  constexpr int operator()(int x) { return ++x; }
----------------
Maybe call this `Increment` and use ` x + 1` instead? It feels a bit weird to use an operation that is typically used to mutate state (in-place increment) in the context of a `transform_view`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/types.h:13
+
+struct View : std::ranges::view_base {
+  int start;
----------------
This is a `ContiguousView`, not just a view, right?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/types.h:19
+  constexpr View& operator=(View&&) = default;
+  constexpr friend int* begin(View& view) noexcept { return view.ptr + view.start; }
+  constexpr friend int* begin(View const& view) noexcept { return view.ptr + view.start; }
----------------
Why are we making all those operations unconditionally `noexcept`? I don't think that's part of the spec for a `contiguous_range`, right? We should keep our archetypes minimal - that's the whole point of archetypes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056