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

Thu Jun 10 05:14:30 PDT 2021

gchatelet marked 2 inline comments 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:
> gchatelet wrote:
> > 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.
> > There is an additional design point [...] When iterating enums we want end to be included.
> 
> Yuck. I //had// missed that. `seq_inclusive` seems like a decent solution to me. If you really wanted to be explicit //everywhere//, you could have a `llvm::halfopen_seq` and an `llvm::closed_seq`... but I assume that's a bad idea because the half-open version is used all over the place and we like the shortness of its name?
> Another option would be a "fluent interface" via member function:
> ```
> for (int i : llvm::seq<int>(0, 10)) { /* loop from 0 to 9 */ }
> for (int i : llvm::seq<int>(1, 10).inclusive()) { /* loop from 1 to 10 */ }
> ```
> Watch out, though: the trivial implementation of `.inclusive()` would lead to integer overflow in `llvm::seq<int>(INT_MIN, INT_MAX).inclusive()` and so you'd have to think about what you wanted to do there: make it work, at the cost of complicated implementation? make it runtime-assert-fail? just ignore the potential overflow? find a simple implementation I'm not thinking of off the top of my head? ;)
> 
> I definitely agree that we //do not want// `seq<E>(x, y)` to iterate over one more element than `seq<underlying_type_t<E>>(x, y)`; the enum-ness of the type parameter should affect only the enum-ness of the `value_type`, definitely not the number of things iterated over.
> 
> Btw, some enumeration types do have "one-past-the-end" values, as in
> ```
> enum E { Apple, Orange, Banana, MaxFruit = Banana+1 };
> for (E fruit : llvm::halfopen_seq<E>(Apple, MaxFruit)) { /* is correct */ }
> ```
> and of course all integer types do have "closed" range forms as well:
> ```
> for (int i : llvm::closed_seq<int>(INT_MIN, INT_MAX)) { /* is correct */ }
> for (int i : llvm::seq<int>(INT_MIN, INT_MAX).inclusive()) { /* is correct */ }
> ```
> 
> > There is an additional design point [...] When iterating enums we want end to be included.
> 
> Yuck. I //had// missed that. `seq_inclusive` seems like a decent solution to me. If you really wanted to be explicit //everywhere//, you could have a `llvm::halfopen_seq` and an `llvm::closed_seq`... but I assume that's a bad idea because the half-open version is used all over the place and we like the shortness of its name?

I count about 150 call sites most of them are exclusive so yeah, I'd prefer to keep the most used version short.

> Another option would be a "fluent interface" via member function:
> ```
> for (int i : llvm::seq<int>(0, 10)) { /* loop from 0 to 9 */ }
> for (int i : llvm::seq<int>(1, 10).inclusive()) { /* loop from 1 to 10 */ }
> ```

I usually like fluent interfaces but it has to be omnipotent (setting a value). The way I implemented it would lead to bugs if client calls `llvm::seq<int>(1, 10).inclusive().inclusive()`.
There are ways to prevent it (adding more state to iota_range) but I feel like handling it as a separate function works quite well.

> Watch out, though: the trivial implementation of `.inclusive()` would lead to integer overflow in `llvm::seq<int>(INT_MIN, INT_MAX).inclusive()` and so you'd have to think about what you wanted to do there: make it work, at the cost of complicated implementation? make it runtime-assert-fail? just ignore the potential overflow? find a simple implementation I'm not thinking of off the top of my head? ;)

I'll go with runtime assert.

> I definitely agree that we //do not want// `seq<E>(x, y)` to iterate over one more element than `seq<underlying_type_t<E>>(x, y)`; the enum-ness of the type parameter should affect only the enum-ness of the `value_type`, definitely not the number of things iterated over.

Yeah they ought to be orthogonal.

> Btw, some enumeration types do have "one-past-the-end" values, as in
> ```
> enum E { Apple, Orange, Banana, MaxFruit = Banana+1 };
> for (E fruit : llvm::halfopen_seq<E>(Apple, MaxFruit)) { /* is correct */ }
> ```
> and of course all integer types do have "closed" range forms as well:
> ```
> for (int i : llvm::closed_seq<int>(INT_MIN, INT_MAX)) { /* is correct */ }
> for (int i : llvm::seq<int>(INT_MIN, INT_MAX).inclusive()) { /* is correct */ }
> ```

Let me add more tests to make sure these cases are handled.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900