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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:237-243
 template <typename ValueT> auto seq(ValueT Begin, ValueT End) {
+  static_assert(!std::is_enum<ValueT>::value,
+                "Use enum_seq when iterating enumerations");
+  static_assert(std::is_integral<ValueT>::value,
+                "ValueT must be an integral type");
   return iota_range<ValueT>(std::move(Begin), std::move(End));
 }
----------------
I don't think it makes sense to force the caller to decide between `seq` and `enum_seq` spellings. If I were implementing this facility, I'd just give `iota_range` a second template type parameter:
```
template<class V, class U>
struct iota_range_iterator {
    using value_type = V;
    V operator*() const { return static_cast<V>(value_); }
    ~~~
private:
    U value_;
};
```
and then here instantiate `iota_range<EnumType, std::underlying_type_t<EnumType>>`.
On line 250 below, you make an `iota_range_iterator<IterableEnum<ValueE>>`, whose `operator*` gives back values of type `IterableEnum<ValueE>` instead of `ValueE` — that's going to be really confusing to the caller. I wouldn't do that.
```
template <class T, std::enable_if_t<std::is_integral<T>::value, int> = 0>
auto seq(T Begin, T End) {
  return iota_range_iterator<T, T>(Begin, End);
}

template <class T, std::enable_if_t<std::is_enum<T>::value, int> = 0>
auto seq(T Begin, T End) {
  using U = std::underlying_type_t<T>;
  return iota_range_iterator<T, U>(U(Begin), U(End));
}
```
Notice you don't need `std::move` because these types are integral.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:43
     constexpr EVT(MVT::SimpleValueType SVT) : V(SVT) {}
+    constexpr EVT(IterableEnum<MVT::SimpleValueType> SVT) : V(SVT) {}
     constexpr EVT(MVT S) : V(S) {}
----------------
Get rid of `IterableEnum` as I propose above, and then you can get rid of this new overload. Ditto throughout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900