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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
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));
 }
----------------
Quuxplusone wrote:
> 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.
Thx for helping me taking a step back.
I originally wanted to separate concerns (iteration and typing) but the fact that `IterableEnum<T>` in not `T` is problematic indeed. The abstraction is leaking.

Now conflating the two orthogonal aspect make the implementation a bit more complex but I think it's worth it.

There is an additional design point that you may have missed. `seq` has the C++ iterator semantic, i.e. the `end` value is exclusive.
When iterating enums we want `end` to be included.
That's a problem with typed enums as user can't write `seq(Enum::First, Enum::Last + 1)`. We are not allowed to do arithmetic on typed enums (and TBH it's cumbersome to write).

I've //fixed// this by adding another method `seq_inclusive` but I'm not too happy about the naming. Suggestions welcome.
Adding a different method does not compose well with overloading `seq` based on `is_integral`/`is_enum` so I moved the type resolution inside `iota_range` directly. Let me know what you think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900