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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
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.");
----------------
cjdb wrote:
> Since Clang supports `__int128` and acknowledges that as an integral type (as does libc++), we should fix this to account for that too.
As we discussed offline, I have concerns about portability here. 

I'd be open to it, but let's do it in it's own patch so we can discuss more thoroughly. 


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


================
Comment at: libcxx/include/__ranges/iota_view.h:369-370
+} // namespace ranges
+
+namespace views {
+namespace __iota {
----------------
cjdb wrote:
> Please move this into namespace `ranges`.
Done in a separate patch. 


================
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)))
+    {
----------------
cjdb wrote:
> What's wrong with having an `auto` return type?
(As discussed offline) We want this for SFINAE.


================
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:
> 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.
See discord discussion. 


================
Comment at: libcxx/include/__ranges/iota_view.h:237
+      {
+        return __i += __n;
+      }
----------------
cjdb wrote:
> 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?
I'll send something to the lwg chair. 


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/compare.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
cjdb wrote:
> 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.
I just deleted eq.pass.cpp and test not-equals here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396