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

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

Quuxplusone 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.");
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900