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

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:37
 
-private:
-  struct Forward {
-    static void increment(T &V) { ++V; }
-    static void decrement(T &V) { --V; }
-    static void offset(T &V, difference_type Offset) { V += Offset; }
-    static T add(const T &V, difference_type Offset) { return V + Offset; }
-    static difference_type difference(const T &A, const T &B) { return A - B; }
-  };
-
-  struct Reverse {
-    static void increment(T &V) { --V; }
-    static void decrement(T &V) { ++V; }
-    static void offset(T &V, difference_type Offset) { V -= Offset; }
-    static T add(const T &V, difference_type Offset) { return V - Offset; }
-    static difference_type difference(const T &A, const T &B) { return B - A; }
-  };
-
-  using Op = std::conditional_t<!IsReversed, Forward, Reverse>;
-
-public:
+struct StrongInt {
   // default-constructible
----------------
`OverflowCheckingInt` ?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:45
   // destructible
-  ~iota_range_iterator() = default;
+  ~StrongInt() = default;
 
----------------
Is there any reason to make the dtor user-provided ? This makes the class non trivially copyable.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:52
+    StrongInt Result;
+    if (__builtin_add_overflow(Value, Offset, &Result.Value))
+      assertOutOfBounds();
----------------
use `MathExtras`'s `AddOverflow` ?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:82
+
+  // Integral constructor, asserts if Value cannot be represented as intmax_t.
+  template <typename Integral, typename std::enable_if_t<
----------------
so `uintmax_t` is no longer supported ? This should be noted somewhere. I'm wondering whether this is an issue or not...


================
Comment at: llvm/include/llvm/ADT/Sequence.h:228
+/// To prevent overflow, `End` must be different from INTMAX_MAX if T is signed
+/// (resp. UINTMAX_MAX if T is unsigned).
+template <typename T> auto seq_inclusive(T Begin, T End) {
----------------
This is no longer relevant, right ?

Also, add a note about signedness ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106279