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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:203
+  auto result = seq(Begin, End);
+  result.End = *++result.end();
+  return result;
----------------
Quuxplusone wrote:
> 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);
> ```
`Begin` and `End` are not iterators but values (BTW I renamed them to avoid confusion), this is needed because of `rbegin()`/`rend()` must be constructed from values.

Now this //can// be handled by adding an additional member but then it gets really confusing when creating the reverse iterators:
```
  value_type Begin;
  value_type End;
  bool Inclusive;

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

  auto begin() const { return const_iterator(Begin, false); }
  auto end() const { return const_iterator(End, Inclusive); }

  auto rbegin() const { return const_reverse_iterator(*--end(), false); }
  auto rend() const { return const_reverse_iterator(*--begin(), false); } // <- false here is confusing

```


================
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)),
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900