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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: CaseyCarter.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/iota_view.h:31-38
+// REVIEW-NOTE: Will be its own patch. Not up for review.
+struct unreachable_sentinel_t {
+  template<weakly_incrementable _Iter>
+  friend constexpr bool operator==(unreachable_sentinel_t, const _Iter&) noexcept { return false; }
+};
+
+inline constexpr unreachable_sentinel_t unreachable_sentinel{};
----------------
This can go away now too.


================
Comment at: libcxx/include/__ranges/iota_view.h:41
+namespace ranges {
+  // TODO: Add a test where diff type size is smaller than short (both int and not) and larger.
+
----------------
This can go away.


================
Comment at: libcxx/include/__ranges/iota_view.h:380-390
+    template<class _Start>
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr auto operator()(_Start&& __start) const {
+      return ranges::iota_view(_VSTD::forward<_Start>(__start));
+    }
+
+    template<class _Start, class _Bound>
----------------
Those are not expression-equivalent as they are required to by http://eel.is/c++draft/range.iota#overview-2. Please also add tests that we're SFINAE friendly (which would fail right now).


================
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>;
----------------
zoecarver wrote:
> tcanens wrote:
> > ldionne wrote:
> > > 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?
> > This seems to me to be a "we'll protect you from accidentally shooting yourself in your foot, but not from intentionally pointing your gun at your foot and pulling the trigger" case. Mixing signs isn't inherently incorrect, so if people want to do it explicitly they can, but the deduction guide forces the type to be spelled out so that the mixedness is obvious.
> > 
> > It's more of an Eric/Casey question though.
> > This seems to me to be a "we'll protect you from accidentally shooting yourself in your foot, but not from intentionally pointing your gun at your foot and pulling the trigger" case. Mixing signs isn't inherently incorrect, so if people want to do it explicitly they can, but the deduction guide forces the type to be spelled out so that the mixedness is obvious.
> 
> This logic makes sense. I suppose that also explains why the deduction guide is here in the first place (and we don't just remove the `type_identity_t`s from the `_Start`/`_Bound` ctor). As always, thanks for interpreting the Standard for us :)
Ping @CaseyCarter, is that really the intent here?

@zoecarver For the time being, can you please make sure those are tested? I'd like a test that fails if we add the constraints to the constructor, and one that fails if we remove them from the deduction guide.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/member_typedefs.compile.pass.cpp:93
+    static_assert(std::same_as<Iter::value_type, int>);
+    static_assert(std::same_as<Iter::difference_type, IntDiffT>);
+  }
----------------



================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/member_typedefs.compile.pass.cpp:109
+    static_assert(std::same_as<Iter::value_type, long long>);
+    static_assert(std::same_as<Iter::difference_type, long long>); // TODO: is this valid? What type should be used here?
+  }
----------------
As you said live, I think this is valid per https://eel.is/c++draft/range.iota.view#1.3. I would suggest this:

```
// comment with link to spec
static_assert(sizeof(Iter::difference_type) >= sizeof(long long));
static_assert(std::is_signed_v<Iter::difference_type>);
LIBCPP_STATIC_ASSERT(std::same_as<Iter::difference_type, long long>);
```


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/minus.pass.cpp:24-28
+using IntDiffT = std::conditional_t<
+  sizeof(int) == sizeof(long),
+  long long,
+  long
+>;
----------------



================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/sentinel/minus.pass.cpp:57
+
+// 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
----------------
This should go away now. And the comment above with `!MinusInvocable<int>` should be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396