[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:159
+      // This assertion forbids overflow of `PastEndValue`.
+      assert(End != std::numeric_limits<raw_type>::max() &&
+             "Forbidden End value for seq_inclusive.");
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900