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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__ranges/iota_view.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
ldionne wrote:
> Can you also add `views::iota` in this patch? It's just a CPO, so it doesn't require the range adaptor stuff (which is close to being done BTW).
I will do this later tonight or tomorrow morning.


================
Comment at: libcxx/include/__ranges/iota_view.h:66
+    iter_difference_t<_Start>,
+    typename __get_wider_signed<_Start>::type
+  >;
----------------
ldionne wrote:
> This `_If` is too eager. It will fail if you pass a non-integer-like-type to it that is larger than `sizeof(long long)`, because you will eagerly instantiate `__get_wider_signed_integer` here and fail in the `static_assert`. This can be fixed by changing to this (not super pretty because we lack `iter_difference`, but oh-well):
> 
> ```
> template<class _Start>
> using _IotaDiffT = typename _If<
>   (!integral<_Start> || sizeof(iter_difference_t<_Start>) > sizeof(_Start)),
>   type_identity<iter_difference_t<_Start>>,
>   __get_wider_signed<_Start>
> >::type;
> ```
> 
> Can you add a test with a non-integer type with a size that's larger than `long long` and confirm that it fails? If it does not fail, we'll learn something interesting. If it does, we can fix it.
>  error: static_assert failed due to requirement 'sizeof(BigType) <= sizeof(long long)' "Found integer-like type that is bigger than largest integer like type."


================
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>();
----------------
Quuxplusone wrote:
> 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`.
> 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.

Is that standard? Are we supposed to support types larger than long? That is why I dropped it from this test. (I should have made a comment, though). 


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/sentinel/minus.pass.cpp:48
+
+// TODO: it seems odd that this is the *only* place we are required to have an input_or_output_iterator.
+// Example: https://godbolt.org/z/WGc9xoqcb
----------------
ldionne wrote:
> Instead, can we add tests to confirm that this *doesn't* work? Can we file a LWG issue to relax this requirement? If this is indeed issue-worthy, our test is going to fail when we fix it, as it should.
Test added. I'll file the LWG issue tomorrow.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/types.h:147
+
+struct NotDecrementable {
+  using difference_type = int;
----------------
Quuxplusone wrote:
> 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?
While I suppose we could, the problem is we need to somehow get a buffer to forward_iterator and then we can't use it to test a default constructible iterator. I think this is a bit simpler. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396