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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

Please add range concept conformance tests.



================
Comment at: libcxx/include/__ranges/iota_view.h:44
+
+      static_assert(sizeof(_Int) <= sizeof(long long),
+        "Found integer-like type that is bigger than largest integer like type.");
----------------
Since Clang supports `__int128` and acknowledges that as an integral type (as does libc++), we should fix this to account for that too.


================
Comment at: libcxx/include/__ranges/iota_view.h:94
+        _If<incrementable<_Start>, forward_iterator_tag,
+    /*Else*/input_iterator_tag>>>;
+
----------------
Keep the conditions and the results in distinct columns please.


================
Comment at: libcxx/include/__ranges/iota_view.h:240
+      {
+        return __i -= __n;
+      }
----------------
This should match `operator+=` or vice versa.


================
Comment at: libcxx/include/__ranges/iota_view.h:369-370
+} // namespace ranges
+
+namespace views {
+namespace __iota {
----------------
Please move this into namespace `ranges`.


================
Comment at: libcxx/include/__ranges/iota_view.h:377
+      noexcept(noexcept(ranges::iota_view(_VSTD::forward<_Start>(__start))))
+            -> decltype(ranges::iota_view(_VSTD::forward<_Start>(__start)))
+    {
----------------
What's wrong with having an `auto` return type?


================
Comment at: libcxx/include/__ranges/iota_view.h:386
+      noexcept(noexcept(ranges::iota_view(_VSTD::forward<_Start>(__start), _VSTD::forward<_Bound>(__bound))))
+            -> decltype(ranges::iota_view(_VSTD::forward<_Start>(__start), _VSTD::forward<_Bound>(__bound)))
+    {
----------------
Ditto.


================
Comment at: libcxx/include/__ranges/iota_view.h:392-395
+
+inline namespace __cpo {
+  inline constexpr auto iota = __iota::__fn{};
+}
----------------
Up to you as to whether or not you wanna do it in this patch or separately, but please add the following to `<ranges>`:
```
_LIBCPP_BEGIN_NAMESPACE_STD

namespace views = namespace ranges::views;

_LIBCPP_END_NAMESPACE_STD
```

Now that we've got something in `ranges::views`, this is worth adding ASAP.


================
Comment at: libcxx/include/__ranges/iota_view.h:257-260
+          if (__y.__value_ > __x.__value_) {
+            return difference_type(-difference_type(__y.__value_ - __x.__value_));
+          }
+          return difference_type(__x.__value_ - __y.__value_);
----------------
zoecarver wrote:
> cjdb wrote:
> > 
> The conditional expression here does not seem easier to read. I could see an argument for it to be the same difficulty, but to me it is harder to read than the if statement. I don't see a reason to change this. 
We're at a bit of an impasse then, because I find the if-statement approach to be the less-readable one.


================
Comment at: libcxx/include/__ranges/iota_view.h:355-361
+        if (__value_ < 0) {
+          if (__bound_ < 0) {
+            return _VSTD::__to_unsigned_like(-__value_) - _VSTD::__to_unsigned_like(-__bound_);
+          }
+          return _VSTD::__to_unsigned_like(__bound_) + _VSTD::__to_unsigned_like(-__value_);
+        }
+        return _VSTD::__to_unsigned_like(__bound_) - _VSTD::__to_unsigned_like(__value_);
----------------
zoecarver wrote:
> cjdb wrote:
> > Yes, I inverted the first condition and moved it to the top.
> Same as above, but this is even harder to read. I really think nested ternaries are an anti pattern, especially when there is a lot going on in each condition. If it was just a variable, that would be one thing, but there are operators inside operators inside operators here, and that makes this very hard to parse. What are we gaining by using the conditional operator here? 
> especially when there is a lot going on in each condition

The conditions are very simple.

>  I really think nested ternaries are an anti pattern

The reason I inverted the expression and moved it to the top was to flatten the expression. In its current state, there the expressions aren't nested (but the standard wording is nested).

> What are we gaining by using the conditional operator here?

It's flat and it's a single expression that can be reasoned about. The way the proposes conditional expression is structured is the same as my rewrite of `iterator_concept` and similar to Louis' rewrite of `__get_wideer_signed`.
By collecting these if-statements into a single expression, we transform imperative code into declarative code, which reduces complexity, and we get to rely on pattern matching:
```
case 1    -> result 1
case 2    -> result 2
otherwise -> default result
```


================
Comment at: libcxx/include/__ranges/iota_view.h:237
+      {
+        return __i += __n;
+      }
----------------
Quuxplusone wrote:
> 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.
Since it's an effects-equivalent-to, I'm not sure if we're allowed to change this at all without first filing an LWG issue (which I'd be okay with). @tcanens any input?


================
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_),
----------------
zoecarver wrote:
> cjdb wrote:
> > ldionne wrote:
> > > That would match the spec better.
> > This suggestion is too general: we're deliberately restricting this to integers and pointers because general reachability is not detectable.
> What does "general reachability is not detectable" mean?
> 
> I think this is fine because the standard says:
> > Preconditions: Bound denotes unreachable_­sentinel_­t or bound is reachable from value. When W and Bound model totally_­ordered_­with, then bool(value <= bound) is true.
> 
Being able to detect whether an iterator can reach a sentinel is not something that we can always check. However, given that precondition, I think it's okay in this case.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/compare.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
zoecarver wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > 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. 
> > I count four assertions for an `==`?
> It's just a sanity check to show that we're starting from the same place. If you feel strongly I can remove them.
Oh, in that case please rename it to `inequality.pass.cpp`, since I thought this was checking all the comparison operations, not just inequalities.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/views_iota.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
You'll also need to add tests for `std::ranges::views::iota`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396