[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:177-178
+    assert(!Inclusive ||
+           End != T(std::numeric_limits<
+                      typename iterator::underlying_type>::max()) &&
+               "Forbidden End value for seq_inclusive.");
----------------
Quuxplusone wrote:
> I believe teeechnically this is still UB, because if the enumeration has no specified underlying type, and its enumerator values are e.g. `enum E { A=1, B=2, C=3 }`, then it's UB to evaluate `E(4)`. (It basically works like a bit-field: you can evaluate `E(mask)` for any `mask` that doesn't use bits beyond the highest enumerator's bits.) But in practice I think this is fine.
> For readability, I would still prefer to see it with an `if`:
> ```
> if (Inclusive)
>   assert(End != T(std::numeric_limits<typename iterator::underlying_type>::max()));
> ```
> Although now you've got me confused again about why we're storing `BeginValue` and `PastEndValue` as `value_type` (i.e. `E`) instead of `underlying_type`. How does your `size()` method work when `value_type` is an enum type? I think data "at rest" should always be stored in the underlying integer type; the only time it should ever be converted to `value_type` (i.e. `E`) is when the iterator is being dereferenced.
> I believe teeechnically this is still UB, because if the enumeration has no specified underlying type, and its enumerator values are e.g. `enum E { A=1, B=2, C=3 }`, then it's UB to evaluate `E(4)`. (It basically works like a bit-field: you can evaluate `E(mask)` for any `mask` that doesn't use bits beyond the highest enumerator's bits.) But in practice I think this is fine.

Thx! So I'm now moving all the logic in the `underlying type` world.
Technically `End + 1` is implementation defined for signed types but it's guarded by the assertion so it's a //logical error// before being //implementation defined//.


> Although now you've got me confused again about why we're storing `BeginValue` and `PastEndValue` as `value_type` (i.e. `E`) instead of `underlying_type`. How does your `size()` method work when `value_type` is an enum type? I think data "at rest" should always be stored in the underlying integer type; the only time it should ever be converted to `value_type` (i.e. `E`) is when the iterator is being dereferenced.

Thx a lot for challenging me on this one. You are correct that we can use `underlying_type` instead of `value_type` directly into `iota_range`.
I've moved things around: I think it's much better now. Let me know if you think this can be improved further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900