[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:159
+      // This assertion forbids overflow of `PastEndValue`.
+      assert(End != std::numeric_limits<raw_type>::max() &&
+             "Forbidden End value for seq_inclusive.");
----------------
gchatelet wrote:
> Quuxplusone wrote:
> > gchatelet wrote:
> > > courbet wrote:
> > > > So you can't `iota_range` and enum with `uint8_t` underlying type and 256 values ? That seems a bit artificial.
> > > > Maybe we could always use `intmax_t` (resp. `uintmax_t` for unsigned underlying types) as raw_type ?
> > > > So you can't `iota_range` and enum with `uint8_t` underlying type and 256 values ? That seems a bit artificial.
> > > > Maybe we could always use `intmax_t` (resp. `uintmax_t` for unsigned underlying types) as raw_type ?
> > > 
> > > Fair enough. I'm slightly concerned about integer promotion/conversion rules but it //should be safe// (famous last words).
> > Well, now I'm wondering why we don't just use `uintptr_t` all the time. Why does the storage type //ever// have to be signed `intptr_t`?
> > I think the rationale has to do with ensuring that `r.begin() < r.end()`; but maybe that just indicates that `Op::difference` is computing its return value via the wrong expression.
> > But anyway, I continue to be uninterested in blocking this at this point. :)
> > 
> > Please do verify that the test failures in the latest run aren't your fault (e.g. by rebasing and letting the tests run again) before committing.
> > Well, now I'm wondering why we don't just use `uintptr_t` all the time. Why does the storage type //ever// have to be signed `intptr_t`?
> 
> So there are enums that explicitly use negative values (e.g. [[ https://github.com/llvm/llvm-project/blob/aa5c09bead260a6008ed7e92a1ee7b1023703b40/llvm/include/llvm/ADT/FloatingPointMode.h#L44 | FloatingPointMode.h:44 ]])  so I think it's reasonable to allow negative values.
> Also, I think it's a valid use case to use seq with negative values as well (e.g. `seq<int>(-10, 0)`).
> 
> > I think the rationale has to do with ensuring that `r.begin() < r.end()`; but maybe that just indicates that `Op::difference` is computing its return value via the wrong expression.
> > But anyway, I continue to be uninterested in blocking this at this point. :)
> 
> Thx a lot for the time you spent on the review. Highly appreciate it.
> 
> 
> > Please do verify that the test failures in the latest run aren't your fault (e.g. by rebasing and letting the tests run again) before committing.
> 
> Yes.
For the record, I was talking about the storage type (`raw_type`) being unsigned, not the value type (`value_type`). I certainly agree that the `value_type` needs to be able to be signed or unsigned, enum or integral. But the reason you ever need anything besides `uintptr_t` as the `raw_type` is much subtler and harder to see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900