[libcxx-commits] [PATCH] D107396: [libcxx][ranges] Add `ranges::iota_view`.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

Mostly LGTM, but a few blockers before I'm happy to give it a green light. A few more comments below.

- Can you please implement `std::ranges::views::iota` as well? That one doesn't need a closure, so we can*  get away with adding it in this patch.
- Don't forget the range concept conformance test.
- You've got a 2.5K log file at the end of this; I don't think that's supposed to be there.



================
Comment at: libcxx/include/__ranges/iota_view.h:64
+  using _IotaDiffT = _If<
+    (!is_integral_v<_Start> || sizeof(iter_difference_t<_Start>) > sizeof(_Start)),
+    iter_difference_t<_Start>,
----------------
Not that I super care, but why is this `is_integral_v` and not `integral`?


================
Comment at: libcxx/include/__ranges/iota_view.h:98
+  class iota_view : public view_interface<iota_view<_Start, _Bound>> {
+    struct __iterator : public __iota_iterator_category<_Start> {
+      friend class iota_view;
----------------
I really find it cluttering that the iterator is defined inside the range. Took me a whole two minutes to realise I was no longer reviewing `iota_view`, but rather `iota_view::__iterator`. (Two mins is probably a slight exaggeration, but it took me longer than I'd have liked.)

Can we please move the definition out-of-line to improve readability?


================
Comment at: libcxx/include/__ranges/iota_view.h:105-117
+      using iterator_concept = _If<
+        __advanceable<_Start>,
+        random_access_iterator_tag,
+        _If<
+          __decrementable<_Start>,
+          bidirectional_iterator_tag,
+          _If<
----------------



================
Comment at: libcxx/include/__ranges/iota_view.h:122
+      __iterator() requires default_initializable<_Start> = default;
+      constexpr explicit __iterator(_Start __value) : __value_(_VSTD::move(__value)) {}
+
----------------
Since this type is exposition-only, I think we can make this constructor private. That'll lead to fewer users attempting to declare an iterator outright.


================
Comment at: libcxx/include/__ranges/iota_view.h:131-133
+      }
+      constexpr void operator++(int) { ++*this; }
+      constexpr __iterator operator++(int) requires incrementable<_Start> {
----------------



================
Comment at: libcxx/include/__ranges/iota_view.h:186-190
+      friend constexpr bool operator==(const __iterator& __x, const __iterator& __y)
+        requires equality_comparable<_Start>
+      {
+        return __x.__value_ == __y.__value_;
+      }
----------------
I'm surprised this can't be just `= default;`


================
Comment at: libcxx/include/__ranges/iota_view.h:244-247
+          if (__y.__value_ > __x.__value_) {
+            return difference_type(-difference_type(__y.__value_ - __x.__value_));
+          }
+          return difference_type(__x.__value_ - __y.__value_);
----------------
I'd also like to see conditional operator use, but the salient feature of the suggested edit is the added else-clause.


================
Comment at: libcxx/include/__ranges/iota_view.h:249
+        }
+        return __x.__value_ - __y.__value_;
+      }
----------------



================
Comment at: libcxx/include/__ranges/iota_view.h:261
+      __sentinel() = default;
+      constexpr explicit __sentinel(_Bound __bound) : __bound_(_VSTD::move(__bound)) {}
+
----------------
Suggest making this private to limit user-touchability.


================
Comment at: libcxx/include/__ranges/iota_view.h:284
+  public:
+    iota_view() requires default_initializable<_Start> = default;
+
----------------
I know the standard has this, but I don't think it's strictly necessary?


================
Comment at: libcxx/include/__ranges/iota_view.h:289
+    constexpr iota_view(type_identity_t<_Start> __value, type_identity_t<_Bound> __bound)
+      : __value_(_VSTD::move(__value)), __bound_(_VSTD::move(__bound)) {}
+
----------------
I'd really like to see the precondition partially applied, particularly for integers, which are a very common use-case for `iota_view`. Same with the above constructor.


================
Comment at: libcxx/include/__ranges/iota_view.h:317-318
+    constexpr auto size() const
+      requires (same_as<_Start, _Bound> && __advanceable<_Start>) || (integral<_Start> && integral<_Bound>) ||
+                 sized_sentinel_for<_Bound, _Start>
+    {
----------------
It took me editing this to realise that `sized_sentinel_for` doesn't have parens at all.


================
Comment at: libcxx/include/__ranges/iota_view.h:320-327
+      if constexpr (__integer_like<_Start> && __integer_like<_Bound>) {
+        if (__value_ < 0) {
+          if (__bound_ < 0) {
+            return __to_unsigned_like(-__value_) - __to_unsigned_like(-__bound_);
+          }
+          return __to_unsigned_like(__bound_) + __to_unsigned_like(-__value_);
+        }
----------------
I'd honestly prefer a conditional expression here.


================
Comment at: libcxx/include/__ranges/iota_view.h:329
+      }
+      return __to_unsigned_like(__bound_ - __value_);
+    }
----------------
Please put this in an else-clause to account for constexpr instantiation..


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/compare.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please add commented-out tests for `<=>`, or a FIXME, or both. We also still need to test `!=` is `not (x == y)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396