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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 10 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/iota_view.h:122
+      __iterator() requires default_initializable<_Start> = default;
+      constexpr explicit __iterator(_Start __value) : __value_(_VSTD::move(__value)) {}
+
----------------
cjdb wrote:
> 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.
It also means it's impossible to test. 

But more importantly: the standard explicitly stated that this type must be default constructible. If users shouldn't care about it being default constructible, why do that? Why not leave it as an implementation detail? There's nothing about iota_view that requires this iterator to be default constructible. 


================
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_);
----------------
cjdb wrote:
> I'd also like to see conditional operator use, but the salient feature of the suggested edit is the added else-clause.
We can discuss further about the conditional operator. 


================
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)) {}
+
----------------
cjdb wrote:
> 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.
Just to make sure we're on the same page: you want `if constexpr (in-debug-mode and is-valid-operation) _LIBCPP_ASSERT(ranges::less_equal(__value, __bound));`? Done.


================
Comment at: libcxx/include/__ranges/iota_view.h:329
+      }
+      return __to_unsigned_like(__bound_ - __value_);
+    }
----------------
cjdb wrote:
> Please put this in an else-clause to account for constexpr instantiation..
It seems like all compilers that we support, at least, are OK with this. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396