[PATCH] D103900: [llvm] Add enum iteration to Sequence

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

Quuxplusone added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:87-91
+    // `index_begin` is unsigned(~0), so effectively iterating ~0, 0, 1, 2...
+    // This is why we need to use `SetIdx != EndIdx` instead of `SetIdx <
+    // EndIdx`.
+    for (unsigned SetIdx = AL.index_begin(), EndIdx = AL.index_end();
+         SetIdx != EndIdx; ++SetIdx) {
----------------
lebedev.ri wrote:
> Quuxplusone wrote:
> > LGTM, but per @lebedev.ri 's comment in D83351, I think this would benefit from a follow-up patch that turns `index_begin` and `index_end` into `signed int`s.
> > Style nit: Personally I'd rather see "`unsigned(-1)`" and "`-1, 0, 1...`", than "`unsigned(~0)`" and "`~0, 0, 1...`".
> This change NACK.
> It should probably be something like
> ```
> for (unsigned SetIdx : seq<int>(AL.index_begin(), AL.index_end())) {
> ```
> or
> ```
> for (unsigned SetIdx : seq((int)AL.index_begin(), (int)AL.index_end())) {
> ```
I approve of `seq((int)AL.index_begin(), (int)AL.index_end())` but not `seq<int>(AL.index_begin(), AL.index_end())` — I don't want people writing explicit template arguments all over the place.
Regardless, this would still benefit from a followup patch to actually change the type of `index_begin` so that we don't //have// to cast it on every use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900