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

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

Quuxplusone added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:171-180
+    assert(Begin <= End);
+    // When `Inclusive && End == std::numeric_limits<T>::max()`
+    // NextValue(End) wraps around and PastEndValue is 0. This is not a problem
+    // for iteration as for-loop stopping condition relies on equality
+    // comparison and same wrap around mechanism occurs.
+    //
+    // That said in this particular case iterator difference and comparison is
----------------
Hmm. I suggest
```
{
  assert(Begin <= End);
  // This assertion forbids iota_range(0, UINT_MAX, true)
  // which would otherwise be treated as an empty range,
  // and iota_range(1, UINT_MAX, true) which would otherwise
  // report that s.begin() > s.end().
  // iota_range(INT_MIN, INT_MAX, true) will have already caused
  // undefined overflow, but this assertion will likely catch it too.
  if (Inclusive)
    assert(End < PastEndValue);
}
```

FWIW, I like your renaming of `Begin` to `BeginValue` etc.


================
Comment at: llvm/unittests/ADT/SequenceTest.cpp:77-84
+    // Inclusive
+    EXPECT_THAT(seq_inclusive<T>(min, min + 1), ElementsAre(min, min + 1));
+    EXPECT_THAT(seq_inclusive<T>(max - 1, max), ElementsAre(max - 1, max));
+    // Inclusive Reverse
+    EXPECT_THAT(reverse(seq_inclusive<T>(min, min + 1)),
+                ElementsAre(min + 1, min));
+    EXPECT_THAT(reverse(seq_inclusive<T>(max - 1, max)),
----------------
gchatelet wrote:
> Quuxplusone wrote:
> > I'd also test `reverse(seq_inclusive(min, min))`.
> > You shouldn't need the `<T>` anywhere on lines 72-84.
> Unfortunately I do because `+ 1` and `- 1` trigger integer promotion which makes both arguments of different types.
> I can introduce variables though.
Ah, I see. (FWIW, I'd bikeshed the names to `minp1` and `maxm1`, but whatever.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900