[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:194
 
-template <typename ValueT> auto seq(ValueT Begin, ValueT End) {
-  return iota_range<ValueT>(Begin, End);
+/// Iterate over an integral/enum type from Begin to End exclusive.
+template <typename T> auto seq(T Begin, T End) {
----------------
I would say "from Begin to End, exclusive of End" — or more colloquially but clearer IMO, "from Begin up to but not including End."


================
Comment at: llvm/include/llvm/ADT/Sequence.h:203
+  auto result = seq(Begin, End);
+  result.End = *++result.end();
+  return result;
----------------
gchatelet wrote:
> Quuxplusone wrote:
> > Phab somehow ate my comment here.
> > This should be just `return seq(Begin, static_cast<T>(End + 1));`
> > and then in the constructor of `iota_range`, please `assert(Begin <= End)` just as a sanity check.
> > Phab somehow ate my comment here.
> > This should be just `return seq(Begin, static_cast<T>(End + 1));`
> 
> Because `End` may be a scoped enum `End + 1` is not always a valid statement.
> Trying to do the arithmetic here will yield the same problems that `IterableEnum` was supposed to solve.
> The best we could do here would be something along those lines:
> 
> ```
> return seq(Begin, static_cast<T>(static_cast<detail::StorageType<T>::type>(End) + 1));
> ```
> 
> Which, IMHO, is redoing most of the work that has been moved to `iota_range_iterator` (hence the use of iterators to bump the values)
Yeah, I guess since `End + 1` doesn't work for scoped enums, I'm willing to live with `*++result.end()`.

Or... Maybe `iota_range` should just take three arguments? Does this work?
```
  explicit iota_range_iterator(T Value, bool plus1)
      : Value(static_cast<U>(Value) + plus1) {}

  explicit iota_range(T Begin, T End, bool inclusive)
      : Begin(Begin, false), End(End, inclusive) {
    assert(Begin <= End);
  }

  // seq
  return iota_range<T>(Begin, End, false);

  // seq_inclusive
  return iota_range<T>(Begin, End, true);
```


================
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)),
----------------
I'd also test `reverse(seq_inclusive(min, min))`.
You shouldn't need the `<T>` anywhere on lines 72-84.


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:1554
 TEST(ConstantRange, ICmp) {
-  for (auto Pred : seq<unsigned>(CmpInst::Predicate::FIRST_ICMP_PREDICATE,
-                                 1 + CmpInst::Predicate::LAST_ICMP_PREDICATE))
-    ICmpTestImpl((CmpInst::Predicate)Pred);
+  for (auto Pred : seq_inclusive<CmpInst::Predicate>(
+           CmpInst::Predicate::FIRST_ICMP_PREDICATE,
----------------
You shouldn't need the explicit template argument here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900