[PATCH] D102679: [llvm] Make Sequence reverse-iterable

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:48
+  };
+  using Op = std::conditional_t<IsForward, Forward, Backward>;
 
----------------
Quuxplusone wrote:
> Naming nit: When I saw `Forward` in an iterator context, I assumed you were talking about forward iterators versus bidirectional iterators, and was going to say "technically, bidirectional iterators are //also// forward iterators." But on second glance I see that by `IsForward` you really mean `IsNotReversed`. I suggest `s/IsForward/!IsReversed/g` and `s/Backward/Reverse/g`.
> 
> Orthogonally, I didn't realize until `operator[]` that by `value_range` you meant not just "`operator*` returns by value," but actually `iota_range`; and I wonder if that would be a good renaming.
@gchatelet wrote:
> how about renaming seq into iota then? In a second NFC patch possibly to keep it simple.

FWIW, I would oppose that. I mean, `iota` //is// established C++ jargon — that's why I used it above when I needed one word to explain my realization — but it's still //jargon//; I think `seq` captures the intent pretty nicely. It's like the shell utility [`seq`](https://en.wikipedia.org/wiki/Seq_(Unix)).

> We may also provide an overloaded version that implicitly starts at 0 (e.g. iota(5) -> 0, 1, 2, 3, 4). More than half of the call sites use 0 explicitly as a first parameter.

I would //extremely strongly// oppose a one-argument overload, if the name were `iota`, because `std::views::iota(5)` is actually `5, 6, 7, ...`. (This is a huge pain point and I think there's a proposal to retroactively kill off the one-argument `iota` from the Standard.)

If the name is kept as `seq`, I would mildly oppose a one-argument overload.

If the name is changed to e.g. `range` or `xrange` (a la Python), then the one-argument overload might make sense — but it would still be unnecessary, so probably provide some-cost-and-no-benefit, so probably not worth doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679