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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 2 inline comments as done.
gchatelet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:171-180
+    assert(Begin <= End);
+    // When `Inclusive && End == std::numeric_limits<T>::max()`
+    // NextValue(End) wraps around and PastEndValue is 0. This is not a problem
+    // for iteration as for-loop stopping condition relies on equality
+    // comparison and same wrap around mechanism occurs.
+    //
+    // That said in this particular case iterator difference and comparison is
----------------
Quuxplusone wrote:
> Hmm. I suggest
> ```
> {
>   assert(Begin <= End);
>   // This assertion forbids iota_range(0, UINT_MAX, true)
>   // which would otherwise be treated as an empty range,
>   // and iota_range(1, UINT_MAX, true) which would otherwise
>   // report that s.begin() > s.end().
>   // iota_range(INT_MIN, INT_MAX, true) will have already caused
>   // undefined overflow, but this assertion will likely catch it too.
>   if (Inclusive)
>     assert(End < PastEndValue);
> }
> ```
> 
> FWIW, I like your renaming of `Begin` to `BeginValue` etc.
> Hmm. I suggest
> ```
> {
>   assert(Begin <= End);
>   // This assertion forbids iota_range(0, UINT_MAX, true)
>   // which would otherwise be treated as an empty range,
>   // and iota_range(1, UINT_MAX, true) which would otherwise
>   // report that s.begin() > s.end().
>   // iota_range(INT_MIN, INT_MAX, true) will have already caused
>   // undefined overflow, but this assertion will likely catch it too.
>   if (Inclusive)
>     assert(End < PastEndValue);
> }
> ```

The assert didn't trigger in the case of `iota_range(INT_MIN, INT_MAX, true) ` so I used `numeric_limits`.
Tests are passing.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900