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

Tue Jun 29 18:22:26 PDT 2021

cjdb requested changes to this revision.
cjdb added a subscriber: CaseyCarter.
cjdb added a comment.
This revision now requires changes to proceed.

- Please add a new entry to the module map.
- Please add iterator and range conformance tests (I think you've already completed some of the iterator conformance tests in a roundabout fashion).

================
Comment at: libcxx/include/__ranges/transform_view.h:125-141
+template<class, class>
+struct __iterator_category_base {};
+
+template<forward_range _View, class _Fn>
+struct __iterator_category_base<_View, _Fn> {
+  using _Cat = typename iterator_traits<iterator_t<_View>>::iterator_category;
+
----------------
This should be in `__iterator/type_traits.h` (or be renamed to something that's `transform_view::iterator`-specific).

================
Comment at: libcxx/include/__ranges/transform_view.h:167
+    : __current_(_VSTD::move(__current)), __parent_(_VSTD::addressof(__parent)) {}
+  constexpr __iterator(__iterator<false> __i)
+    requires _Const && convertible_to<iterator_t<_View>, iterator_t<_Base>>
----------------
I can't remember, but there might be a technical reason why the standard says `!Const` and not `false`. cc @tcanens and @CaseyCarter for fact checking.

================
Comment at: libcxx/include/__ranges/transform_view.h:258-262
+//   friend constexpr auto operator<=>(const __iterator& __x, const __iterator& __y)
+//     requires random_access_range<_Base> && three_way_comparable<iterator_t<_Base>>
+//   {
+//     return __x.__current_ <=> __y.__current_;
+//   }
----------------
Quuxplusone wrote:
> What's the deal with this commented-out `operator<=>`?
D103478 hasn't been merged yet. @zoecarver can you please add a TODO so we remember to come back and uncomment it ASAP?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:50-51
+
+template<>
+constexpr bool std::ranges::enable_view<std::vector<int>> = true;
+
----------------
This smells like a bug. `std::vector` isn't a `view` and any attempts to treat it as such are IFNDR.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:39
+auto withRandom(R range, Fn func = Fn()) {
+  return std::ranges::transform_view(range, std::bind_front(func, std::rand()));
+}
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > I don't understand what's going on here with `std::bind_front`, nor the defaulted function argument `func = Fn()`, but surely our unit tests should not depend on the behavior of `rand()`.
> > Please switch to e.g. `[](int x) { return x + 1; }` and remove the `bind` and default-function-argument stuff.
> > 
> The tests in this file are not testing any one thing in particular. They're meant to be a few general use cases that we might see in the wild. I suppose you're right about random, though. I'll find another function. 
If there's a call to `bind_front` then there needs to be a `<functional>` inclusion.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:50-54
+template<>
+constexpr bool std::ranges::enable_view<std::vector<int>> = true;
+
+template<>
+constexpr bool std::ranges::enable_view<std::string_view> = true;
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > I assume lines 50-54 were here for polyfill purposes; they can be removed now that `enable_view` is in master, right?
> Unfortunately not yet. We still need to actually //use// `enable_view` on main.
[[ https://github.com/llvm/llvm-project/blob/main/libcxx/include/string_view#L662 | This is already in main ]]. I suspect you need to rebase.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/compare.pass.cpp:35
+  assert(iter1     >= iter2);
+  // TODO: operator<=> when three_way_comparable is implemented.
+
----------------
Can you add the tests and just  have them commented out for now please?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/plus_minus.pass.cpp:26
+  iter1 += 4;
+  assert((iter1 + 1).base() == globalBuff + 5);
+  assert((1 + iter1).base() == globalBuff + 5);
----------------
Please add a test checking that `(iter + n) - n) == iter` and `(iter - n) + n) == iter`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/requirements.compile.pass.cpp:20-32
+template<class V, class F>
+concept IterMinusMinus = requires(std::ranges::iterator_t<std::ranges::transform_view<V, F>> iter) {
+  iter--;
+};
+static_assert(IterMinusMinus<BidirectionalView, IncrementConst>);
+static_assert(!IterMinusMinus<ForwardView, IncrementConst>);
+
----------------
Is this not just `static_assert(std::bidirectional_iterator<std::ranges::transform_view<BidirectionalView, IncrementConst>>);`? Similarly for below with `std::random_access_iterator`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/requirments.compile.pass.cpp:50
+concept IterSubscript = requires(std::ranges::iterator_t<std::ranges::transform_view<V, F>> iter) {
+  iter[0];
+};
----------------
Quuxplusone wrote:
> Concepts pedantry: If you use `1` instead of `0` here, you'll have one extra defense (`0` can be a null pointer constant, `1` can't).
We should really be using a variable, not a constant.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/types.pass.cpp:24
+template<class V, class F>
+concept HasIterConcept = requires { typename std::ranges::transform_view<V, F>::iterator_category; };
+
----------------
`HasIterConcept` implies `iterator_concept`. Perhaps `HasIteratorCategory`?

================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference.pass.cpp:47
         ASSERT_SAME_TYPE(decltype(*opt), X&);
+        LIBCPP_STATIC_ASSERT(noexcept(*opt));
         // ASSERT_NOT_NOEXCEPT(*opt);
----------------
Can you please move the `optional` changes to their own commit? I'd prefer to keep this one solely focussed on `transform_view`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056