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

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

I probably shouldn't be the //only// person to accept this PR, but it looks fine to me at this point.



================
Comment at: llvm/include/llvm/ADT/Sequence.h:28
 
-template <typename T, bool IsReversed> struct iota_range_iterator {
+template <typename U, typename T, bool IsReversed> struct iota_range_iterator {
   using iterator_category = std::random_access_iterator_tag;
----------------
Nits: Please add a newline before `struct`; and consider renaming/reordering the parameters so that `T` comes before `U`. I think that should be easy and mechanical (if tedious), because neither `T` nor `U` is deduced nor defaulted.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:196
+  auto rbegin() const { return const_reverse_iterator(PastEndValue - 1); }
+  auto rend() const { return const_reverse_iterator(BeginValue - 1); }
 
----------------
Note that `BeginValue-1` might be UB; e.g. `seq(INT_MIN, 0).rend()`. But I think it's fine to ignore this corner case.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:214
+/// Iterate over an integral/enum type from Begin to End inclusive.
+/// `End` is not allowed to be
+template <typename T> auto seq_inclusive(T Begin, T End) {
----------------
"End is not allowed to be"... what?


================
Comment at: llvm/unittests/ADT/SequenceTest.cpp:54
 
+enum class CharEnum : char { A = 1, B, C, D, E, LAST = E };
+
----------------
Nit: I don't see why you bother defining `LAST` when you could just say `E` below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900