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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
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_);
----------------
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. 


================
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_);
----------------
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? 


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



================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/compare.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
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.


================
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
----------------
tcanens wrote:
> zoecarver wrote:
> > @tcanens thoughts?
> `sized_sentinel_for` has an opt-out, and any relaxation would need to both respect that and perhaps supply its own opt-out mechanism. That gets hairy pretty quickly and doesn't seem to be worth the extra complexity.
I will never understand the decision matrix that leads to the Committee thinking some things are worth the complexity and others aren't :P 

Anyway, thanks for the info about this, Tim. I guess I won't file an LWG issue, then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396