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

Tue Aug 10 12:23:50 PDT 2021

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.

================
Comment at: libcxx/include/__ranges/iota_view.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
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).

================
Comment at: libcxx/include/__ranges/iota_view.h:27
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
----------------
Not necessary.

================
Comment at: libcxx/include/__ranges/iota_view.h:43-60
+  template<class _Int>
+  struct __get_wider_signed {
+    static_assert(sizeof(_Int) <= sizeof(long long),
+      "Found integer-like type that is bigger than largest integer like type.");
+    using type = long long;
+  };
+
----------------

================
Comment at: libcxx/include/__ranges/iota_view.h:63
+  template<class _Start>
+  using _IotaDiffT = _If<
+    (!integral<_Start> || sizeof(iter_difference_t<_Start>) > sizeof(_Start)),
----------------
Can you please add tests that checks the member typedefs of `iota_view::iterator` itself? In particular, I'd like to confirm that `iterator_difference_t` is deduced correctly when `W::difference_type` has sizes less than `short`, `int`, `long`, etc.

Note per our discussion: If it's not possible to have `W::difference_type` both be an integral type and be smaller than `short`, we should perhaps add a `static_assert` to keep us honest about it. But as we discussed, I think it is possible by having `W::difference_type` be `char`.

================
Comment at: libcxx/include/__ranges/iota_view.h:66
+    iter_difference_t<_Start>,
+    typename __get_wider_signed<_Start>::type
+  >;
----------------
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.

================
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>,
----------------
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.

================
Comment at: libcxx/include/__ranges/iota_view.h:282
+      __sentinel() = default;
+      constexpr explicit __sentinel(_Bound __bound) : __bound_(_VSTD::move(__bound)) {}
+
----------------
Nit: make sure you test explicitness.

================
Comment at: libcxx/include/__ranges/iota_view.h:312
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit iota_view(_Start __value) : __value_(_VSTD::move(__value)) { }
+
----------------
Do you test the explicitness by checking `!std::is_convertible`? If not, please do.

================
Comment at: libcxx/include/__ranges/iota_view.h:318-320
+      if constexpr ((integral<_Start> || is_pointer_v<_Start>) &&
+                    (integral<_Bound> || is_pointer_v<_Bound>)) {
+        _LIBCPP_ASSERT(ranges::less_equal()(__value_, __bound_),
----------------
That would match the spec better.

================
Comment at: libcxx/include/__ranges/iota_view.h:331
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr iota_view(__iterator __first, unreachable_sentinel_t __last)
+      requires same_as<_Bound, unreachable_sentinel_t>
----------------
This is very nitpicky, but I would rather define this as `iota_view(__iterator __first, _Bound __last)` even though those are the same, simply because it matches the spec more closely and IMO it is easier to read that way (see http://eel.is/c++draft/range.iota#view-11.2).

================
Comment at: libcxx/include/__ranges/iota_view.h:362
+    {
+      if constexpr (__integer_like<_Start> && __integer_like<_Bound>) {
+        if (__value_ < 0) {
----------------
Can you please add tests at boundary conditions like `numeric_limits<T>::max()` & friends?

================
Comment at: libcxx/include/__ranges/iota_view.h:375-378
+  template<class _Start, class _Bound>
+    requires (!__integer_like<_Start> || !__integer_like<_Bound> ||
+              (__signed_integer_like<_Start> == __signed_integer_like<_Bound>))
+  iota_view(_Start, _Bound) -> iota_view<_Start, _Bound>;
----------------
Possible spec issue:

I'm kind of wondering why we don't define the constructor like this instead, and avoid defining a deduction guide altogether:

```
constexpr iota_view(W value, Bound bound) 
    requires (!is-integer-like<W> || !is-integer-like<Bound> ||
             (is-signed-integer-like<W> == is-signed-integer-like<Bound>));
```

As specified in the Standard, we have the following situation, which seems to be weird and inconsistent:

```
iota_view<signed, unsigned> view(x, y); // works
iota_view view((signed)x, (unsigned)y); // doesn't work
```

I think both of them should not work.

@tcanens is that intentional?

================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/begin.pass.cpp:11
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: gcc-10
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
----------------
Not necessary anymore, we don't support GCC 10.

I have a patch to remove them everywhere, but it would be nice if we stopped introducing those in new patches from now on.

================
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
----------------
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.

================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/types.h:1
+#ifndef TEST_STD_RANGES_RANGE_FACTORIES_RANGE_IOTA_VIEW_TYPES_H
+#define TEST_STD_RANGES_RANGE_FACTORIES_RANGE_IOTA_VIEW_TYPES_H
----------------
Missing license.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396