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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Minor nits.



================
Comment at: libcxx/include/__ranges/iota_view.h:230-231
+
+  //     friend constexpr bool operator<=>(const __iterator& __x, const __iterator& __y)
+  //       requires totally_ordered<_Start> && three_way_comparable<_Start>;
+
----------------
Nit: You might as well provide the commented-out definition of this function's body. Also, `bool` should be `auto`.


================
Comment at: libcxx/include/__ranges/iota_view.h:237
+      {
+        return __i += __n;
+      }
----------------
This matches the Standard reference implementation, but it might be worth noting that `return a += b;` does a copy-construct of parameter `a`, whereas `a += b; return a;` would do only a move-construct.


================
Comment at: libcxx/include/__ranges/iota_view.h:369
+      }
+      return __to_unsigned_like(__bound_ - __value_);
+    }
----------------
Style nit: Even though the arguments are going to be `integral`, I'd like to see the usual ADL-proofing on this function call: `_VSTD::__to_unsigned_like(...)` throughout.


================
Comment at: libcxx/include/__ranges/iota_view.h:98
+  class iota_view : public view_interface<iota_view<_Start, _Bound>> {
+    // Not all compilers handle bool the same way. For consistency we just ban it.
+    static_assert(!std::same_as<_Start, bool>,
----------------
ldionne wrote:
> I think the correct location to fix this issue is in whatever concept currently accepts `bool` when it shouldn't, which is `weakly_incrementable` IIUC. So I would suggest fixing that in `weakly_incrementable` with a `&& !same_as<bool, T>`, along with a test that `bool` doesn't model `weakly_incrementable` (which we must be lacking right now), and also a TODO comment to remove the special case for `bool` once Clang has been fixed.
+1 what Louis said; and anyway, since `iota_view` is supposed to be constrained, the mechanism needs to be SFINAE-friendly (as Louis's suggestion is). E.g. we expect https://godbolt.org/z/fcx3r61PT
```
template<class T> iota_view<T> f(int);
template<class T> void f(...);
int main() { f<bool>(42); }
```
to be unambiguous because SFINAE.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/begin.pass.cpp:45-46
+  testType<SomeInt>();
+  testType<signed long>();
+  testType<unsigned long>();
+  testType<int>();
----------------
I notice a conspicuous lack of `long long` and `unsigned long long` here.
Notice that `iota_view<long long>::difference_type` needs to be `__int128_t` or some other integer-like class type suitable for holding the signed difference between `LLONG_MIN` and `LLONG_MAX`.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/types.h:147
+
+struct NotDecrementable {
+  using difference_type = int;
----------------
I bet you could use `forward_iterator<int*>` out of `test_iterators.h` here, instead of having to invent your own type. You can make an `iota_view<forward_iterator<int*>>`, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396